Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 11:35:10 GMT, Daniel Fuchs wrote: >> - use `String.equalsIgnoreCase()` instead of hand-written code relying on >> `String.charAt()` >> - use `String.compareToIgnoreCase()` instead of hand-written code relying on >> `String.charAt()` >> - drop branches that are never executed >> - drop unused argument from `URI.resolvePath()` >> - simplify String-related operations > > src/java.base/share/classes/java/net/URI.java line 1921: > >> 1919: return sn - tn; >> 1920: int val = 0; >> 1921: int n = Math.min(sn, tn); > > Can we drop this change? I wouldn't like `java.net.URI` to gratuitously > trigger the loading of the Math class. > For the rest of the proposed changes, I will need to study them carefully and > that may take some time. > Thanks! Not sure it matters that much? `Math` is already unconditionally loaded on bootstrap, though slightly after `URI` in the bootstrap sequence. - PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8281298: Revise the creation of unmodifiable list
On Sat, 5 Feb 2022 20:29:50 GMT, Xue-Lei Andrew Fan wrote: > In [JDK-8281289](https://bugs.openjdk.java.net/browse/JDK-8281289), the > SSLParameters class was updated with the patch: > > - return Collections.unmodifiableList(new > ArrayList<>(sniNames.values())); > + return List.copyOf(sniNames.values()); > > However, there's a small compatibility risk with this change, > `List.copyOf(...).contains(null)` will throw NPE while > `Collections.unmodifiableList(...).contains(null)` won't. It may be not > worthy of the risk although the impact may be minimal. > > This update will re-use the Collections.unmodifiableList() methods, but > re-org the code for better performance. This looks like an appropriate solution which avoids the minor compatibility risk introduced by the previous change - and you might even end up being more efficient both when setting and reading the names/matchers. (Since we're going down the route of optimizing this: the `type` of a `SNIServerName` is constrained to a value between 0 and 255, so there's likely a small micro-optimization opportunity in using a `BitSet` for the duplicate checking rather a `ArrayList`.. A `BitSet(256)` should have lower memory use and the existence check will run in O(1) instead of O(n) time. Might not be worthwhile to optimize these setters, though..) - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7359
Re: RFR: 8281289: Improve with List.copyOf
On Fri, 4 Feb 2022 23:02:21 GMT, Xue-Lei Andrew Fan wrote: > Please review this trivial code clean up, for a little bit better performance. There's a small compatibility risk with this change, e.g., `List.copyOf(...).contains(null)` will throw NPE while `Collections.unmodifiableList(...).contains(null)` won't. If we accept that compatibility risk (which should probably be decided by CSR) it might also make sense to use `List.of()` for the empty case, which will reduce the number of List implementation classes returned from the API. - PR: https://git.openjdk.java.net/jdk/pull/7356
Integrated: 8272626: Avoid C-style array declarations in java.*
On Wed, 18 Aug 2021 10:07:35 GMT, Claes Redestad wrote: > C-style array declarations generate noisy warnings in IDEs, et.c. This patch > cleans up all java.* packages. > > (Copyrights intentionally not updated due the triviality of most changes) This pull request has now been integrated. Changeset: 30b0f820 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/30b0f820cec12b6da62229fe78a528ab3ad0d134 Stats: 140 lines in 54 files changed: 0 ins; 0 del; 140 mod 8272626: Avoid C-style array declarations in java.* Reviewed-by: dfuchs, alanb - PR: https://git.openjdk.java.net/jdk/pull/5161
Re: RFR: 8272626: Avoid C-style array declarations in java.*
On Wed, 18 Aug 2021 10:07:35 GMT, Claes Redestad wrote: > C-style array declarations generate noisy warnings in IDEs, et.c. This patch > cleans up all java.* packages. > > (Copyrights intentionally not updated due the triviality of most changes) Thanks for reviewing, Daniel and Alan! - PR: https://git.openjdk.java.net/jdk/pull/5161
RFR: 8272626: Avoid C-style array declarations in java.*
C-style array declarations generate noisy warnings in IDEs, et.c. This patch cleans up all java.* packages. (Copyrights intentionally not updated due the triviality of most changes) - Commit messages: - Avoid C-style array declarations in java packages Changes: https://git.openjdk.java.net/jdk/pull/5161/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5161=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272626 Stats: 140 lines in 54 files changed: 0 ins; 0 del; 140 mod Patch: https://git.openjdk.java.net/jdk/pull/5161.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5161/head:pull/5161 PR: https://git.openjdk.java.net/jdk/pull/5161
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Yes, while I don't know exactly which changes resolved JDK-6764325, it's clear
from the microbenchmarks added for #2102 that it's no longer an issue - at
least not in the mainline.
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8267840: Improve URLStreamHandler.parseURL() [v3]
On Mon, 5 Jul 2021 13:42:15 GMT, Сергей Цыпанов
wrote:
>> There is an optimization opportunity for the widespread use-case when a
>> resource is read from classpath using
>> `getClass().getClassLoader().getResource()` or
>> `getClass().getClassLoader().getResourceAsStream()`.
>>
>> Pay attention to lines starting from 261. In case I run something like
>>
>> var props =
>> getClass().getClassLoader().getResource("/application.properties");
>>
>> I get into the if-else block starting from 251 and here 'separator' variable
>> is an empty String. In this case we can skip 'separator' from concatenation
>> chain and use `String.concat()` as there are only two items concatenated.
>>
>> In the opposite case `separator` variable is `"/"` and at the same time
>> `ind` variable is `-1`. This means that expression `path.substring(0, ind +
>> 1)` always returns an empty String and again can be excluded from
>> concatenation chain allowing usage of `String.concat()` which allows to
>> dodge utilization of `StringBuilder` (here `StringConcatFactory` is not
>> available, see https://github.com/openjdk/jdk/pull/3627)
>>
>> In the next else-block, starting from 274, again, `String.concat()` is
>> applicable.
>>
>> In another if-else block, starting from 277, when id is 0 again
>> path.substring(0, ind) returns empty String making concatenation pointless
>> and avoidable.
>>
>> There are also some other minor clean-ups possible regarding constant
>> conditions (lines 252 and 161).
>>
>> The change allows to reduce significantly resource look-up costs for a very
>> wide-spread case:
>>
>> @State(Scope.Benchmark)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ClassGetResourceBenchmark {
>> private final Class clazz = getClass();
>>
>> @Benchmark
>> public URL getResource() {
>> return clazz.getResource("/application.properties");
>> }
>> }
>>
>> The change allows to reduce memory consumption significantly:
>>
>> before
>>
>> Benchmark Mode
>> Cnt Score Error Units
>> ClassGetResourceBenchmark.getResource avgt
>> 100 1649,367 ± 5,904 ns/op
>> ClassGetResourceBenchmark.getResource:·gc.alloc.rateavgt
>> 100 619,204 ± 2,413 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.alloc.rate.norm avgt
>> 100 1339,232 ± 4,909B/op
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Eden_Space avgt
>> 100 627,192 ± 74,972 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Eden_Space.norm avgt
>> 100 1356,681 ± 162,354B/op
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Survivor_Space avgt
>> 100 0,119 ± 0,100 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Survivor_Space.norm avgt
>> 100 0,257 ± 0,217B/op
>> ClassGetResourceBenchmark.getResource:·gc.count avgt
>> 100 128,000counts
>> ClassGetResourceBenchmark.getResource:·gc.time avgt
>> 100 227,000ms
>>
>> after
>>
>> Benchmark Mode
>> Cnt Score Error Units
>> ClassGetResourceBenchmark.getResource avgt
>> 100 1599,948 ± 4,115 ns/op
>> ClassGetResourceBenchmark.getResource:·gc.alloc.rateavgt
>> 100 358,434 ± 0,922 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.alloc.rate.norm avgt
>> 100 752,016 ± 0,004B/op
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Eden_Space avgt
>> 100 342,778 ± 76,490 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Eden_Space.norm avgt
>> 100 719,264 ± 160,513B/op
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Survivor_Space avgt
>> 100 0,008 ± 0,005 MB/sec
>> ClassGetResourceBenchmark.getResource:·gc.churn.G1_Survivor_Space.norm avgt
>> 100 0,017 ± 0,010B/op
>> ClassGetResourceBenchmark.getResource:·gc.count avgt
>> 10070,000counts
>> ClassGetResourceBenchmark.getResource:·gc.time avgt
>> 100 151,000ms
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8267840: Improve URLStreamHandler.parseURL() - remove unnecessary
> String.concat() call
LGTM
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4526
Re: RFR: 8263561: Re-examine uses of LinkedList [v5]
On Mon, 26 Jul 2021 08:27:20 GMT, Сергей Цыпанов wrote: >> After I've renamed remove branch GitHub for some reason has closed original >> https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > - Merge branch 'master' into 8263561 > ># Conflicts: ># src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java > - Merge branch 'master' into purge-linked-list > - 8263561: Use sized constructor where reasonable > - 8263561: Use interface List instead of particular type where possible > - 8263561: Rename requestList -> requests > - 8263561: Re-examine uses of LinkedList I always approve of removing LinkedLists and Vectors. Using ArrayDeque in AbstractPoller seems like the right choice. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов wrote: >> Non-static classes hold a link to their parent classes, which in many cases >> can be avoided. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8261880: Remove static from declarations of Holder nested classes Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8263658: Use the blessed modifier order in java.base [v2]
On Thu, 18 Mar 2021 17:02:57 GMT, Alex Blewitt wrote: >> Sonar displays a warning message that modifiers should be declared in the >> order listed in the JLS; specifically, that isntead of using `final static` >> the `static final` should be preferred. >> >> This fixes the issues in the `java.base` package for ease of reviewing. >> >> https://sonarcloud.io/project/issues?id=shipilev_jdk=java=false=java%3AS1124 > > Alex Blewitt has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8263658: Use the blessed modifier order in java.base [v2]
On Thu, 18 Mar 2021 17:06:04 GMT, Alex Blewitt wrote: >>> If I have other fixes for different modules, should I file PRs with the >>> same bug number e.g. "8263658: Use the blessed modifier order in >>> java.logging/java.desktop" or should we have separate bug numbers for them? >> >> Separate bug numbers is necessary. Unless you repurpose / widen this PR to >> include more modules. >> >> A word of advice: Avoid git rebase + force push after opening a PR for >> review, since it might mess up the review context. Preferably either merge >> in changes from main, or just keep adding commits without syncing up - the >> system will ensure your patch can be merged in without conflicts. > > I'm happy to either widen the scope of this PR or to submit multiple but I > have to rely on the kindness of strangers to create appropriate bugs in the > system. On the one hand I don't want to cause a lot of noise but on the > other having smaller independent commits (particularly if they hit a lot of > files/modules) seems like a more sensible plan to me. There's some extra churn in splitting it up, sure, but different modules are often maintained by different people so getting changes that span the entire JDK reviewed might actually take you longer. YMMV. I can assist creating RFEs. - PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8263658: Use the blessed modifier order in java.base [v2]
On Thu, 18 Mar 2021 16:51:35 GMT, Alex Blewitt wrote: > If I have other fixes for different modules, should I file PRs with the same > bug number e.g. "8263658: Use the blessed modifier order in > java.logging/java.desktop" or should we have separate bug numbers for them? Separate bug numbers is necessary. Unless you repurpose / widen this PR to include more modules. A word of advice: Avoid git rebase + force push after opening a PR for review, since it might mess up the review context. Preferably either merge in changes from main, or just keep adding commits without syncing up - the system will ensure your patch can be merged in without conflicts. - PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8263658: Use the blessed modifier order in java.base
On Thu, 18 Mar 2021 16:42:39 GMT, Alex Blewitt wrote: >> Yeah, I agree. > > Is that there to indicate a placeholder value that was once used and is kept > for documentation purposes? Should the corresponding JavaDoc be removed as > well? Should I do this in the same commit/PR as this one, or submit a new PR? > Would prefer to avoid conflating fixes if possible so that if one needs to be > reverted we don't revert the related changes. There's another constant with value 3 defined, so I think this is just some left-over. If you prefer separating out the removal to another RFE I'd remove this particular change from this PR. - PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8263658: Use the blessed modifier order in java.base
On Sat, 13 Mar 2021 22:45:30 GMT, Alex Blewitt wrote: > Sonar displays a warning message that modifiers should be declared in the > order listed in the JLS; specifically, that isntead of using `final static` > the `static final` should be preferred. > > This fixes the issues in the `java.base` package for ease of reviewing. > > https://sonarcloud.io/project/issues?id=shipilev_jdk=java=false=java%3AS1124 Marked as reviewed by redestad (Reviewer). src/java.base/share/classes/com/sun/security/ntlm/NTLMException.java line 52: > 50: * from server. > 51: */ > 52: //public static final int DOMAIN_UNMATCH = 3; Maybe this one ought to be removed instead? - PR: https://git.openjdk.java.net/jdk/pull/2993
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 16:35:02 GMT, liach wrote: >> I'll just revert them > > For static methods, since in java language you cannot declare static method > in instance inner classes, I'd say making them static makes more sense > language-wise. Also making them static reduces compiler synthetic instance > field and constructors. Incidentally, Java-the-language allows static methods in inner instance classes since JDK 16. And I'm not sure this was ever a restriction at the JVMS level since we've been generating static methods (using ASM) into these inner instance classes since at least JDK 9. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible
On Tue, 16 Feb 2021 14:30:58 GMT, Сергей Цыпанов
wrote:
> Non-static classes hold a link to their parent classes, which in many cases
> can be avoided.
src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line
192:
> 190:
> 191: /* Placeholder class for DelegatingMethodHandles generated ahead of
> time */
> 192: static final class Holder {}
For the four `Holder` classes in `java.lang.invoke`, the class is generated
when running jlink via `java.lang.invoke.GenerateJLIClassesHelper`. To stay in
sync with the definition you'd have to update that code. Also, since these
`Holder` classes will only contain static methods and are never instantiated
then I'm not sure it matters whether the classes are static or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed [v2]
On Wed, 27 Jan 2021 19:03:38 GMT, Sean Mullan wrote: >> Claes Redestad has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Copyrights >> - Same for Windows > > Marked as reviewed by mullan (Reviewer). @seanjmullan @dfuch @Michael-Mc-Mahon @AlanBateman - thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/2263
Integrated: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed
On Wed, 27 Jan 2021 14:47:08 GMT, Claes Redestad wrote: > 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager > installed This pull request has now been integrated. Changeset: 8fe1323d Author: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/8fe1323d Stats: 10 lines in 2 files changed: 0 ins; 0 del; 10 mod 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed Reviewed-by: alanb, dfuchs, michaelm, mullan - PR: https://git.openjdk.java.net/jdk/pull/2263
Re: RFR: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed [v2]
> 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager > installed Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Copyrights - Same for Windows - Changes: - all: https://git.openjdk.java.net/jdk/pull/2263/files - new: https://git.openjdk.java.net/jdk/pull/2263/files/09e4ff9e..81d3a4d0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2263=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2263=00-01 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2263/head:pull/2263 PR: https://git.openjdk.java.net/jdk/pull/2263
Re: RFR: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed [v2]
On Wed, 27 Jan 2021 16:57:25 GMT, Sean Mullan wrote: > Will you make the same change to > `src/java.base/windows/classes/sun/net/www/protocol/jar/JarFileFactory.java`? Good catch. Let's keep things consistent. :-) - PR: https://git.openjdk.java.net/jdk/pull/2263
Re: RFR: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed
On Wed, 27 Jan 2021 14:54:33 GMT, Alan Bateman wrote: >> 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager >> installed > > Should be okay as the side effect of getPermission is not observable. Make > sure to run all the tests before integrating. @AlanBateman @dfuch thanks for reviewing. Waiting for a tier1+2 test run before integration. - PR: https://git.openjdk.java.net/jdk/pull/2263
RFR: 8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed
8260520: Avoid getting permissions in JarFileFactory when no SecurityManager installed - Commit messages: - Avoid getting permissions when there's no SM installed Changes: https://git.openjdk.java.net/jdk/pull/2263/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2263=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260520 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2263/head:pull/2263 PR: https://git.openjdk.java.net/jdk/pull/2263
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v4]
On Thu, 21 Jan 2021 10:11:11 GMT, Eirik Bjorsnos wrote: >> By moving string splitting and concatenation into the canonizeString >> utility, we can defer allocation until we determine that canonization is >> required. This saves two string allocations and a string concat for the >> common case where canonization is not required. >> >> As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler >> since that's the only call site. >> >> Finally, let's rename the method to canonizalizeString, since canonization >> is an rather unrelated term. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year in affected files since the change is non-trivial Looks good. (You need to cast /integrate again for us to be able to /sponsor) - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL
On Wed, 20 Jan 2021 15:25:18 GMT, eirbjo
wrote:
> By moving string splitting and concatenation into the canonizeString utility,
> we can defer allocation until we determine that canonization is required.
> This saves two string allocations and a string concat for the common case
> where canonization is not required.
>
> As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler since
> that's the only call site.
>
> Finally, let's rename the method to canonizalizeString, since canonization is
> an rather unrelated term.
LGTM - just a small nit that you seem to have forgotten to change canonize to
canonicalize in some places.
src/java.base/share/classes/sun/net/www/protocol/jar/Handler.java line 232:
> 230: }
> 231:
> 232: private static String doCanonize(String file) {
nit: doCanonicalize
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset) [v3]
On Thu, 14 Jan 2021 11:48:16 GMT, Claes Redestad wrote: >> @cl4es SB brings pessimization both for time and memory, try >> `org.openjdk.bench.java.net.URLEncodeDecode`: >> master >> (count) (maxLength) >> (mySeed) Mode Cnt Score Error Units >> testEncodeUTF8 1024 1024 >> 3 avgt 25 8.573 ? 0.023 ms/op >> testEncodeUTF8:?gc.alloc.rate 1024 1024 >> 3 avgt 25 1202.896 ? 3.225 MB/sec >> testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 >> 3 avgt 25 11355727.904 ? 196.249B/op >> testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 >> 3 avgt 25 1203.785 ? 6.240 MB/sec >> testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 >> 3 avgt 25 11364143.637 ? 52830.222B/op >> testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 >> 3 avgt 25 0.008 ? 0.001 MB/sec >> testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 >> 3 avgt 2577.088 ? 9.303B/op >> testEncodeUTF8:?gc.count1024 1024 >> 3 avgt 25 1973.000 counts >> testEncodeUTF8:?gc.time 1024 1024 >> 3 avgt 25 996.000 ms >> >> enc >> (count) (maxLength) >> (mySeed) Mode CntScore Error Units >> testEncodeUTF8 1024 1024 >> 3 avgt 257.931 ? 0.006 ms/op >> testEncodeUTF8:?gc.alloc.rate 1024 1024 >> 3 avgt 25 965.347 ? 0.736 MB/sec >> testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 >> 3 avgt 25 8430590.163 ? 7.213B/op >> testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 >> 3 avgt 25 966.373 ? 5.248 MB/sec >> testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 >> 3 avgt 25 8439563.689 ? 47282.178B/op >> testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 >> 3 avgt 250.007 ? 0.001 MB/sec >> testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 >> 3 avgt 25 60.949 ? 8.405B/op >> testEncodeUTF8:?gc.count1024 1024 >> 3 avgt 25 1715.000 counts >> testEncodeUTF8:?gc.time 1024 1024 >> 3 avgt 25 888.000 ms >> >> stringBuilder >> (count) (maxLength) >> (mySeed) Mode Cnt Score Error Units >> testEncodeUTF8 1024 1024 >> 3 avgt 25 8.115 ? 0.110 ms/op >> testEncodeUTF8:?gc.alloc.rate 1024 1024 >> 3 avgt 25 1259.267 ?16.716 MB/sec >> testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 >> 3 avgt 25 11249391.875 ? 6.552B/op >> testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 >> 3 avgt 25 1259.937 ?17.232 MB/sec >> testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 >> 3 avgt 25 11255413.875 ? 43636.143B/op >> testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 >> 3 avgt 25 0.007 ? 0.001 MB/sec >> testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 >> 3 avgt 2559.461 ? 9.087B/op >> testEncodeUTF8:?gc.count1024 1024 >> 3 avgt 25 2236.000 counts >> testEncodeUTF8:?gc.time 1024 1024 >> 3 avgt 25 1089.000 ms >> The reason seems to be single char `StringBuilder.append()` that apart from >> range check does encoding check and stores `char` as two bytes in `byte[]` >> in ASB > > Surprising, but thanks for checking! No need to merge in changes in master unless there are conflicts or you want to do more changes of your own, they will be merged automatically on integration. But it seems the bots need you to do /integrate again - PR: https://git.openjdk.java.net/jdk/pull/1598
Re: RFR: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset) [v2]
On Thu, 14 Jan 2021 09:31:19 GMT, Сергей Цыпанов wrote: >> Looks good. >> >> I wonder... `CharArrayWriter` is an old and synchronized data structure, and >> since the instance used here isn't shared that synchronization seem useless. >> And since you're now bypassing the `char[]` and going straight for a >> `String` you might get better performance with a `StringBuilder` here? >> (`setLength(0)` instead of `reset()`...) > > @cl4es SB brings pessimization both for time and memory, try > `org.openjdk.bench.java.net.URLEncodeDecode`: > master > (count) (maxLength) > (mySeed) Mode Cnt Score Error Units > testEncodeUTF8 1024 1024 > 3 avgt 25 8.573 ? 0.023 ms/op > testEncodeUTF8:?gc.alloc.rate 1024 1024 > 3 avgt 25 1202.896 ? 3.225 MB/sec > testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 > 3 avgt 25 11355727.904 ? 196.249B/op > testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 > 3 avgt 25 1203.785 ? 6.240 MB/sec > testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 > 3 avgt 25 11364143.637 ? 52830.222B/op > testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 > 3 avgt 25 0.008 ? 0.001 MB/sec > testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 > 3 avgt 2577.088 ? 9.303B/op > testEncodeUTF8:?gc.count1024 1024 > 3 avgt 25 1973.000 counts > testEncodeUTF8:?gc.time 1024 1024 > 3 avgt 25 996.000 ms > > enc > (count) (maxLength) > (mySeed) Mode CntScore Error Units > testEncodeUTF8 1024 1024 > 3 avgt 257.931 ? 0.006 ms/op > testEncodeUTF8:?gc.alloc.rate 1024 1024 > 3 avgt 25 965.347 ? 0.736 MB/sec > testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 > 3 avgt 25 8430590.163 ? 7.213B/op > testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 > 3 avgt 25 966.373 ? 5.248 MB/sec > testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 > 3 avgt 25 8439563.689 ? 47282.178B/op > testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 > 3 avgt 250.007 ? 0.001 MB/sec > testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 > 3 avgt 25 60.949 ? 8.405B/op > testEncodeUTF8:?gc.count1024 1024 > 3 avgt 25 1715.000 counts > testEncodeUTF8:?gc.time 1024 1024 > 3 avgt 25 888.000 ms > > stringBuilder > (count) (maxLength) > (mySeed) Mode Cnt Score Error Units > testEncodeUTF8 1024 1024 > 3 avgt 25 8.115 ? 0.110 ms/op > testEncodeUTF8:?gc.alloc.rate 1024 1024 > 3 avgt 25 1259.267 ?16.716 MB/sec > testEncodeUTF8:?gc.alloc.rate.norm 1024 1024 > 3 avgt 25 11249391.875 ? 6.552B/op > testEncodeUTF8:?gc.churn.G1_Eden_Space 1024 1024 > 3 avgt 25 1259.937 ?17.232 MB/sec > testEncodeUTF8:?gc.churn.G1_Eden_Space.norm 1024 1024 > 3 avgt 25 11255413.875 ? 43636.143B/op > testEncodeUTF8:?gc.churn.G1_Survivor_Space 1024 1024 > 3 avgt 25 0.007 ? 0.001 MB/sec > testEncodeUTF8:?gc.churn.G1_Survivor_Space.norm 1024 1024 > 3 avgt 2559.461 ? 9.087B/op > testEncodeUTF8:?gc.count1024 1024 > 3 avgt 25 2236.000 counts > testEncodeUTF8:?gc.time 1024 1024 > 3 avgt 25 1089.000 ms > The reason seems to be single char `StringBuilder.append()` that apart from > range check does encoding check and stores `char` as two bytes in `byte[]` in > ASB Surprising, but thanks for checking! - PR: https://git.openjdk.java.net/jdk/pull/1598
Re: RFR: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset)
On Thu, 3 Dec 2020 14:29:58 GMT, Сергей Цыпанов
wrote:
> Instead of allocating a copy of underlying array via
> `CharArrayWriter.toCharArray()` and passing it to constructor of String
> String str = new String(charArrayWriter.toCharArray());
> we could call `toString()` method
> String str = charArrayWriter.toString();
> decoding existing char[] without making a copy. This slightly speeds up the
> method reducing at the same time memory consumption for decoding URLs with
> non-latin symbols:
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class UrlEncoderBenchmark {
> private static final Charset charset = Charset.defaultCharset();
> private static final String utf8Url =
> "https://ru.wikipedia.org/wiki/Организация_Объединённых_Наций;; // UN
>
> @Benchmark
> public String encodeUtf8() {
> return URLEncoder.encode(utf8Url, charset);
> }
> }
> The benchmark on my maching give the following output:
> before
> BenchmarkMode Cnt
> ScoreError Units
> UrlEncoderBenchmark.encodeUtf8 avgt 100
> 1166.378 ± 8.411 ns/op
> UrlEncoderBenchmark.encodeUtf8:·gc.alloc.rateavgt 100
> 932.944 ± 6.393 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.alloc.rate.norm avgt 100
> 1712.193 ± 0.005B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Eden_Space avgt 100
> 929.221 ± 24.268 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Eden_Space.norm avgt 100
> 1705.444 ± 43.235B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Survivor_Space avgt 100
> 0.006 ± 0.001 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Survivor_Space.norm avgt 100
> 0.011 ± 0.002B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.count avgt 100
> 652.000 counts
> UrlEncoderBenchmark.encodeUtf8:·gc.time avgt 100
> 334.000 ms
>
> after
> BenchmarkMode Cnt
> ScoreError Units
> UrlEncoderBenchmark.encodeUtf8 avgt 100
> 1058.851 ± 6.006 ns/op
> UrlEncoderBenchmark.encodeUtf8:·gc.alloc.rateavgt 100
> 931.489 ± 5.182 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.alloc.rate.norm avgt 100
> 1552.176 ± 0.005B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Eden_Space avgt 100
> 933.491 ± 24.164 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Eden_Space.norm avgt 100
> 1555.488 ± 39.204B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Survivor_Space avgt 100
> 0.006 ± 0.001 MB/sec
> UrlEncoderBenchmark.encodeUtf8:·gc.churn.G1_Survivor_Space.norm avgt 100
> 0.010 ± 0.002B/op
> UrlEncoderBenchmark.encodeUtf8:·gc.count avgt 100
> 655.000 counts
> UrlEncoderBenchmark.encodeUtf8:·gc.time avgt 100
> 333.000 ms
Looks good.
I wonder... `CharArrayWriter` is an old and synchronized data structure, and
since the instance used here isn't shared that synchronization seem useless.
And since you're now bypassing the `char[]` and going straight for a `String`
you might get better performance with a `StringBuilder` here? (`setLength(0)`
instead of `reset()`...)
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1598
Re: RFR: 8255989: Remove explicitly unascribed authorship from Java source files
On Fri, 6 Nov 2020 20:11:24 GMT, Pavel Rappo wrote: > This PR proposes to remove > 1. JavaDoc `@author` tags with unclear semantics: `@author > unascribed|unattributed|unknown` > 2. A couple of astray Form Feed (a.k.a. FF, `\f`, `0xC`, or `^L`) characters A removed @author tag is a good @author tag - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1100
Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects
On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов
wrote:
> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a
> reason to explicitly instantiate any instance of `Atomic*` class with its
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
> @Benchmark
> public Object defaultValue() {
> return new AtomicInteger();
> }
> @Benchmark
> public Object explicitValue() {
> return new AtomicInteger(0);
> }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark Mode Cnt Score Error Units
> AtomicBenchmark.defaultValue avgt 30 4.778 ± 0.403 ns/op
> AtomicBenchmark.explicitValue avgt 30 11.846 ± 0.273 ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in
> progress we could trivially replace explicit zeroing with default
> constructors gaining some performance benefit with no risk.
>
> I've tested the changes locally, both tier1 and tier 2 are ok.
>
> Could one create an issue for tracking this?
Marked as reviewed by redestad (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/818
Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects
On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов
wrote:
> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a
> reason to explicitly instantiate any instance of `Atomic*` class with its
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
> @Benchmark
> public Object defaultValue() {
> return new AtomicInteger();
> }
> @Benchmark
> public Object explicitValue() {
> return new AtomicInteger(0);
> }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark Mode Cnt Score Error Units
> AtomicBenchmark.defaultValue avgt 30 4.778 ± 0.403 ns/op
> AtomicBenchmark.explicitValue avgt 30 11.846 ± 0.273 ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in
> progress we could trivially replace explicit zeroing with default
> constructors gaining some performance benefit with no risk.
>
> I've tested the changes locally, both tier1 and tier 2 are ok.
>
> Could one create an issue for tracking this?
Filed [8255299](https://bugs.openjdk.java.net/browse/JDK-8255299) for this.
Prefix the name of the PR with "8255299: " and it should pass checks.
-
PR: https://git.openjdk.java.net/jdk/pull/818
Re: RFR 8245308 : Replace ThreadLocalCoders decoder/encoder cache in java.net.URI
On 2020-08-27 16:11, Alan Bateman wrote:
For the micro, I'm curious if iterations is needed.
I'd say the canonical JMH way of doing these might be something as
simple as this:
@Benchmark
public URI encodeURI() throws URISyntaxException {
return new URI("http", "\u00A0", "\u00A0");
}
@Benchmark
public String decodeURI() throws URISyntaxException {
return decoderUri.getPath();
}
Sometimes for very simple micros like this this means the
workload gets inlined so aggressively that you no longer
measure what is likely to happen in the real world, and it's then
not uncommon to force the benchmark method to not be inlined up
into the JMH framework by annotating them with this:
@CompilerControl(CompilerControl.Mode.DONT_INLINE)
See for example org/openjdk/bench/java/util/ImmutableColls.java
If you run with these suggestions you'll only one have encode/decode
per iteration, so you might want to change output to nanoseconds, i.e.:
@OutputTimeUnit(TimeUnit.NANOSECONDS)
Thanks!
/Claes
Re: RFR: 8242186: Reduce allocations in URLStreamHandler.parseURL for some cases (Was: Re: [NEW BUG] Small optimization in URLStreamHandler.parseURL)
On 2020-04-06 13:22, Chris Hegarty wrote: Bug:https://bugs.openjdk.java.net/browse/JDK-8242186 Let me know if there are any concerns, otherwise I'll push once tier1 testing comes back green. Thanks! Thanks all for this nice micro-optimization. The changes look good to me. -Chris. Thanks, Chris! /Claes
RFR: 8242186: Reduce allocations in URLStreamHandler.parseURL for some cases (Was: Re: [NEW BUG] Small optimization in URLStreamHandler.parseURL)
Hi,
(including net-dev@.. since this is in java.net)
I intend to sponsor this trivial patch provided by Christoph:
diff -r 65b345254ca3
src/java.base/share/classes/java/net/URLStreamHandler.java
--- a/src/java.base/share/classes/java/net/URLStreamHandler.java Thu Apr
02 05:44:04 2020 +
+++ b/src/java.base/share/classes/java/net/URLStreamHandler.java Mon Apr
06 12:27:09 2020 +0200
@@ -266,8 +266,8 @@
spec.substring(start, limit);
} else {
-String separator = (authority != null) ? "/" : "";
-path = separator + spec.substring(start, limit);
+path = spec.substring(start, limit);
+path = (authority != null) ? "/" + path : path;
}
} else if (queryOnly && path != null) {
int ind = path.lastIndexOf('/');
Bug: https://bugs.openjdk.java.net/browse/JDK-8242186
Let me know if there are any concerns, otherwise I'll push
once tier1 testing comes back green. Thanks!
/Claes
On 2020-04-05 19:48, Claes Redestad wrote:
Hi Christoph,
looks good and trivial. I'll sponsor.
/Claes
On 2020-04-05 10:57, Christoph Dreis wrote:
Hi,
I just noticed a small optimization opportunity in
URLStreamHandler.parseURL for inputs like the following:
new URL(new URL("file:"), "file:./path/to/some/local.properties");
In those cases there is no authority involved, which makes it possible
to rewrite URLStreamHandler:L269-270:
String separator = (authority != null) ? "/" : "";
path = separator + spec.substring(start, limit);
into the following to avoid unnecessary string concatenations with an
empty string:
path = spec.substring(start, limit);
path = (authority != null) ? "/" + path : path;
I've written a small benchmark with two URLStreamHandler variants (one
with the old parseURL and one with the new):
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class MyBenchmark {
@State(Scope.Benchmark)
public static class ThreadState {
private URL context;
private URLStreamHandler newHandler = new NewURLStreamHandler();
private URLStreamHandler oldHandler = new OldURLStreamHandler();
private String spec =
"file:./path/to/some/application.properties";
public ThreadState() {
try {
context = new URL("file:");
} catch (MalformedURLException ignored) {
}
}
}
@Benchmark
public URL testNew(ThreadState threadState) throws
MalformedURLException {
return new URL(threadState.context, threadState.spec,
threadState.newHandler);
}
@Benchmark
public URL testOld(ThreadState threadState) throws
MalformedURLException {
return new URL(threadState.context, threadState.spec,
threadState.oldHandler);
}
}
The benchmark above yields the following results on my local machine:
Benchmark Mode Cnt
Score Error Units
MyBenchmark.testNew avgt 10
112,655 ± 3,494 ns/op
MyBenchmark.testNew:·gc.alloc.rate avgt 10
1299,558 ± 41,238 MB/sec
MyBenchmark.testNew:·gc.alloc.rate.norm avgt 10
192,015 ± 0,003 B/op
MyBenchmark.testNew:·gc.count avgt 10
98,000 counts
MyBenchmark.testNew:·gc.time avgt 10
105,000 ms
MyBenchmark.testOld avgt 10
128,592 ± 1,183 ns/op
MyBenchmark.testOld:·gc.alloc.rate avgt 10
1612,743 ± 15,792 MB/sec
MyBenchmark.testOld:·gc.alloc.rate.norm avgt 10
272,019 ± 0,001 B/op
MyBenchmark.testOld:·gc.count avgt 10
110,000 counts
MyBenchmark.testOld:·gc.time avgt 10
121,000 ms
In case you think this is worthwhile I would need someone to sponsor
the attached patch for me.
I would highly appreciate that.
Cheers,
Christoph
= PATCH =
diff --git
a/src/java.base/share/classes/java/net/URLStreamHandler.java
b/src/java.base/share/classes/java/net/URLStreamHandler.java
--- a/src/java.base/share/classes/java/net/URLStreamHandler.java
+++ b/src/java.base/share/classes/java/net/URLStreamHandler.java
@@ -266,8 +266,8 @@
spec.substring(start, limit);
} else {
- String separator = (authority != null) ? "/" : "";
- path = separator + spec.substring(start, limit);
+ path = spec.substring(start, limit);
+ path = (authority != null) ? "/" + path : path;
}
} else if (queryOnly && path != null) {
int ind = path.lastIndexOf('/');
Re: RFR[8237480]: Add micros for DatagramSocket send/receive
On 2020-02-14 13:01, Chris Hegarty wrote: http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.03/ LGTM. +1 /Claes
Re: RFR[8237480]: Add micros for DatagramSocket send/receive
Hi, looks reasonable to me. To keep default execution times at a manageable level, I'd recommend excluding some of the 9 values for the size parameter (3-5 values seem reasonable). /Claes On 2020-02-12 16:04, Patrick Concannon wrote: Hi, Could someone please review my webrev for JDK-8237480 'Add micros for DatagramSocket send/receive' ? This change adds some benchmarks for the DatagramSocket::send and DatagramSocket::receive methods. bug: https://bugs.openjdk.java.net/browse/JDK-8237480 webrev: http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.01/ Kind regards, Patrick
Re: RFR: 8228394: Cleanup unused java.net SharedSecrets classes
On 2019-07-19 15:28, Chris Hegarty wrote: Nice cleanup! Reviewed. Thanks for reviewing, Chris! /Claes
Re: RFR: 8228394: Cleanup unused java.net SharedSecrets classes
On 2019-07-19 15:01, Alan Bateman wrote: This looks good to me. Thanks for reviewing, Alan! /Claes
RFR: 8228394: Cleanup unused java.net SharedSecrets classes
Hi, please review this cleanup to remove a few java.net access objects that are no longer in use. Webrev: http://cr.openjdk.java.net/~redestad/8228394/open.00/ Bug:https://bugs.openjdk.java.net/browse/JDK-8228394 (One usage was from sun.misc.ClassLoaderUtil, removed by https://bugs.openjdk.java.net/browse/JDK-8034853, which seems to have forgotten to remove a related and now unused test.jar file. I removed this too and verified all tests pass.) Testing: tier1+2 Thanks! /Claes
Re: RFR: 8227587: Add internal privileged System.loadLibrary
Hi Peter, On 2019-07-17 10:29, Peter Levart wrote: Hi Claes, Am I reading correct that package-private ClassLoader.loadLibrary(Class fromClass, String name, boolean isAbsolute) wouldn't need to consult SecurityManager wasn't it for accessing system properties in private ClassLoader.initializePath(String propName). So perhaps if the two calls to initializePath() in the if block of ClassLoader.loadLibrary() are wrapped with a single doPrivileged(), you wouldn't have to wrap the call to JavaLangAccess.loadLibrary in BootLoader. And since initializePath() would only be called once and then the results cached, you end up with less doPrivileged() wrappings when SecurityManager is present as soon as BootLoader.loadLibrary is called the 2nd time. hmm, fromClass.getClassLoader() also does a security check, though. That said, there's a case for wrapping the initializePath calls in a doPriv of their own, since we now have an implicit requirement that some system library is loaded first (so that sys_paths is initialized) before any user code attempts to load a library of their own... BTW, there is a data race when accessing usr_paths and sys_paths static fields in multi-threaded scenario. Their type is String[] which means that the contents of the arrays could be observed uninitialized (elements being null) in some thread. Both fields should be made volatile (not only sys_paths) as assignment to them can be performed more than once in idempotent racy initialization. .. and we typically seem to be loading at least one library before initializing any user code, which is probably why this has never been an issue. It seems like a good idea to remove this subtle fragility, though. I wonder if we shouldn't "simply" initialize the two paths during bootstrap and put them in final fields. These are pre-existing issues though, so I think I'll take my reviews and push this enhancement for now. :-) Thanks! /Claes Regards, Peter On 7/16/19 3:48 PM, Claes Redestad wrote: Hi, refactored to use a BootLoader::loadLibrary API that is only visible within the java.base module: http://cr.openjdk.java.net/~redestad/8227587/open.03/ I've retained the bridge to ClassLoader::loadLibrary for performance, but without any magic or privilege escalation involved. Moving sys_path / systemNativeLibraries out of ClassLoader seems quite complicated, so I've not attempted that refactoring for this RFE. /Claes On 2019-07-12 20:03, Mandy Chung wrote: Just to recap what we discussed offline: We could introduce BootLoader::loadLibrary to load a system native library for boot loader. sys_path and systemNativeLibraries in ClassLoader implementation are solely for boot loader and it seems cleaner for BootLoader to handle loading of system native libraries where it can be optimized for boot loader. This would require some refactoring. One possible solution is to add a shared secret to simply call package-private ClassLoader::loadLibrary(Class fromClass, String name) method and have BootLoader::loadLibrary to check if SM is present and wrap the call with doPriv; otherwise, just call the shared secret loadLibrary method. Then we can investigate in the future to refactor the native library loading implementation to jdk.internal.loader. what do you think? Mandy On 7/12/19 8:25 AM, Mandy Chung wrote: Claes, Thanks for exploring this. I would like to see if we can avoid introducing an internal @CS method (keep @CSM only for public APIs will help security analysis). There are other alternatives to improve the footprint performance. One idea is java.base and java.desktop each has its own utility method to load library. No change is needed for java.net since only one loadLibrary call. Another idea is to add BootLoader.loadLibrary that skips the security permission overhead and only for boot loader use. It would require refactoring. I think java.base and java.desktop having its own utility method is a simpler solution. Mandy On 7/12/19 7:55 AM, Claes Redestad wrote: Hi, I'm dropping the java.desktop changes, and Mandy has asked me to explore options that does not add a new @CallerSensitive entry point, even to a strongly encapsulated API like JavaLangAccess Easiest of these is to explicitly provide the class context we're calling from - with proper assertions. This is marginally more performant due avoiding the Reflection.getCallerClass, but slightly less convenient. http://cr.openjdk.java.net/~redestad/8227587/open.01/ /Claes On 2019-07-12 15:27, Roger Riggs wrote: Hi Claes, Looks fine. Thanks for the updates, Roger On 7/11/19 10:39 AM, Claes Redestad wrote: Hi Roger, On 2019-07-11 16:10, Roger Riggs wrote: Hi Claes, JavaLangAccess.java: 316: Add @param tag done. System.java: 2282, 2287 Runtime.loadLibrary0 makes a second check for a security manager. Is there any potential gain by calling ClassLoader.loadLibrary directly? None of the internal uses would have a sep
Re: RFR: 8227720: Improve ExtendedSocketOptions initialization
On 2019-07-16 17:11, Vyom Tewari26 wrote: Looks good to me. Vyom Thanks for reviewing, Vyom /Claes
Re: RFR: 8227720: Improve ExtendedSocketOptions initialization
On 2019-07-16 17:01, Chris Hegarty wrote: On 16 Jul 2019, at 15:06, Claes Redestad wrote: Webrev: http://cr.openjdk.java.net/~redestad/8227720/open.00/ This looks good to me Claes. Thanks for doing it. Reviewed. Thanks for reviewing, Chris! /Claes
RFR: 8227720: Improve ExtendedSocketOptions initialization
Hi, a small cleanup and startup optimization for apps that setup and/or use sockets of various kinds, which pulls in ExtendedSocketOptions. By being slightly more eager we avoid a few potentially costly streams (featuring capturing lambdas) that we'd end up calling eagerly from places like AbstractPlainSocketImpl anyhow. Bug:https://bugs.openjdk.java.net/browse/JDK-8227720 Webrev: http://cr.openjdk.java.net/~redestad/8227720/open.00/ Testing: tier1-3 Thanks! /Claes
Re: RFR: 8227587: Add internal privileged System.loadLibrary
Hi, refactored to use a BootLoader::loadLibrary API that is only visible within the java.base module: http://cr.openjdk.java.net/~redestad/8227587/open.03/ I've retained the bridge to ClassLoader::loadLibrary for performance, but without any magic or privilege escalation involved. Moving sys_path / systemNativeLibraries out of ClassLoader seems quite complicated, so I've not attempted that refactoring for this RFE. /Claes On 2019-07-12 20:03, Mandy Chung wrote: Just to recap what we discussed offline: We could introduce BootLoader::loadLibrary to load a system native library for boot loader. sys_path and systemNativeLibraries in ClassLoader implementation are solely for boot loader and it seems cleaner for BootLoader to handle loading of system native libraries where it can be optimized for boot loader. This would require some refactoring. One possible solution is to add a shared secret to simply call package-private ClassLoader::loadLibrary(Class fromClass, String name) method and have BootLoader::loadLibrary to check if SM is present and wrap the call with doPriv; otherwise, just call the shared secret loadLibrary method. Then we can investigate in the future to refactor the native library loading implementation to jdk.internal.loader. what do you think? Mandy On 7/12/19 8:25 AM, Mandy Chung wrote: Claes, Thanks for exploring this. I would like to see if we can avoid introducing an internal @CS method (keep @CSM only for public APIs will help security analysis). There are other alternatives to improve the footprint performance. One idea is java.base and java.desktop each has its own utility method to load library. No change is needed for java.net since only one loadLibrary call. Another idea is to add BootLoader.loadLibrary that skips the security permission overhead and only for boot loader use. It would require refactoring. I think java.base and java.desktop having its own utility method is a simpler solution. Mandy On 7/12/19 7:55 AM, Claes Redestad wrote: Hi, I'm dropping the java.desktop changes, and Mandy has asked me to explore options that does not add a new @CallerSensitive entry point, even to a strongly encapsulated API like JavaLangAccess Easiest of these is to explicitly provide the class context we're calling from - with proper assertions. This is marginally more performant due avoiding the Reflection.getCallerClass, but slightly less convenient. http://cr.openjdk.java.net/~redestad/8227587/open.01/ /Claes On 2019-07-12 15:27, Roger Riggs wrote: Hi Claes, Looks fine. Thanks for the updates, Roger On 7/11/19 10:39 AM, Claes Redestad wrote: Hi Roger, On 2019-07-11 16:10, Roger Riggs wrote: Hi Claes, JavaLangAccess.java: 316: Add @param tag done. System.java: 2282, 2287 Runtime.loadLibrary0 makes a second check for a security manager. Is there any potential gain by calling ClassLoader.loadLibrary directly? None of the internal uses would have a separatorChar. Most of the gain comes from not having to load one-off PA classes or lambdas, but of course avoiding the indexOf and a few indirections are marginally helpful to startup. We should at least assert that the library names are sane, though. I expect most of the files need a copyright update. The script in /make/scripts/update_copyright_year.sh should do the work for modified files. Fixed copyrights and updated in place: http://cr.openjdk.java.net/~redestad/8227587/open.00 Thanks! /Claes
Re: RFR: 8227587: Add internal privileged System.loadLibrary
Hi, I'm dropping the java.desktop changes, and Mandy has asked me to explore options that does not add a new @CallerSensitive entry point, even to a strongly encapsulated API like JavaLangAccess Easiest of these is to explicitly provide the class context we're calling from - with proper assertions. This is marginally more performant due avoiding the Reflection.getCallerClass, but slightly less convenient. http://cr.openjdk.java.net/~redestad/8227587/open.01/ /Claes On 2019-07-12 15:27, Roger Riggs wrote: Hi Claes, Looks fine. Thanks for the updates, Roger On 7/11/19 10:39 AM, Claes Redestad wrote: Hi Roger, On 2019-07-11 16:10, Roger Riggs wrote: Hi Claes, JavaLangAccess.java: 316: Add @param tag done. System.java: 2282, 2287 Runtime.loadLibrary0 makes a second check for a security manager. Is there any potential gain by calling ClassLoader.loadLibrary directly? None of the internal uses would have a separatorChar. Most of the gain comes from not having to load one-off PA classes or lambdas, but of course avoiding the indexOf and a few indirections are marginally helpful to startup. We should at least assert that the library names are sane, though. I expect most of the files need a copyright update. The script in /make/scripts/update_copyright_year.sh should do the work for modified files. Fixed copyrights and updated in place: http://cr.openjdk.java.net/~redestad/8227587/open.00 Thanks! /Claes
Re: RFR: 8227587: Add internal privileged System.loadLibrary
Hi Roger, On 2019-07-11 16:10, Roger Riggs wrote: Hi Claes, JavaLangAccess.java: 316: Add @param tag done. System.java: 2282, 2287 Runtime.loadLibrary0 makes a second check for a security manager. Is there any potential gain by calling ClassLoader.loadLibrary directly? None of the internal uses would have a separatorChar. Most of the gain comes from not having to load one-off PA classes or lambdas, but of course avoiding the indexOf and a few indirections are marginally helpful to startup. We should at least assert that the library names are sane, though. I expect most of the files need a copyright update. The script in /make/scripts/update_copyright_year.sh should do the work for modified files. Fixed copyrights and updated in place: http://cr.openjdk.java.net/~redestad/8227587/open.00 Thanks! /Claes
RFR: 8227587: Add internal privileged System.loadLibrary
Hi, by adding a method to load libraries with privileges to JavaLangAccess, we can simplify a slew of places where we are currently implementing adhoc privileged actions. This is a tiny but measurable gain on a range of startup tests. Webrev: http://cr.openjdk.java.net/~redestad/8227587/open.00 Bug: https://bugs.openjdk.java.net/browse/JDK-8227587 Testing: tier1-3 Thanks! /Claes
Re: RFR: 8225239: Refactor NetworkInterface lookups
Thanks, Daniel! /Claes On 2019-07-05 12:07, Daniel Fuchs wrote: Hi Claes, That looks like a good cleanup and improvement. Thanks for having looked into this! best regards, -- daniel On 04/07/2019 21:20, Claes Redestad wrote: Hi, please review this patch that refactors native java.net.NetworkInterface lookup logic in a few ways to address both pre-existing and recent regressions: Bug: https://bugs.openjdk.java.net/browse/JDK-8225239 Webrev: http://cr.openjdk.java.net/~redestad/8225239/open.01/ - adds a package-private method isBoundInetAddress that shortcuts some of the paths taken by existing methods when all we want to know is whether an IP address is bound to some interface on this system. This allows us to iterate over only IPv4 interfaces if the sought after IP address is an IPv4 address and vice versa for IPv6 addresses. This means a small (1.1x) to large (5x) speedup on a variety of systems, depending on how many IPv4 vs IPv6 interfaces exist on the system.. - refactors how the Windows-specific GetIPAddrTable API is used: current implementation looks up the global IP address table over and over, once per bound address, which means time spent by operations like NetworkInterface.getByInetAddress grows at least quadratically with the number of interfaces on a system. Refactoring these methods so that GetIPAddrTable is called once means a dramatic improvement on some of the API calls, e.g getByInetAddress cost drops from ~100ms to ~10ms on one of our larger test systems. - Added some additional tests and a microbenchmark. Testing: tier1-4 (some still ongoing, but all green so far) Thanks! /Claes
Re: RFR: 8225239: Refactor NetworkInterface lookups
Thanks, Michael! /Claes On 2019-07-05 10:02, Michael McMahon wrote: Claes, This is great work and long overdue. Thanks for taking care of it. - Michael. On 04/07/2019, 20:20, Claes Redestad wrote: Hi, please review this patch that refactors native java.net.NetworkInterface lookup logic in a few ways to address both pre-existing and recent regressions: Bug: https://bugs.openjdk.java.net/browse/JDK-8225239 Webrev: http://cr.openjdk.java.net/~redestad/8225239/open.01/ - adds a package-private method isBoundInetAddress that shortcuts some of the paths taken by existing methods when all we want to know is whether an IP address is bound to some interface on this system. This allows us to iterate over only IPv4 interfaces if the sought after IP address is an IPv4 address and vice versa for IPv6 addresses. This means a small (1.1x) to large (5x) speedup on a variety of systems, depending on how many IPv4 vs IPv6 interfaces exist on the system.. - refactors how the Windows-specific GetIPAddrTable API is used: current implementation looks up the global IP address table over and over, once per bound address, which means time spent by operations like NetworkInterface.getByInetAddress grows at least quadratically with the number of interfaces on a system. Refactoring these methods so that GetIPAddrTable is called once means a dramatic improvement on some of the API calls, e.g getByInetAddress cost drops from ~100ms to ~10ms on one of our larger test systems. - Added some additional tests and a microbenchmark. Testing: tier1-4 (some still ongoing, but all green so far) Thanks! /Claes
RFR: 8225239: Refactor NetworkInterface lookups
Hi, please review this patch that refactors native java.net.NetworkInterface lookup logic in a few ways to address both pre-existing and recent regressions: Bug:https://bugs.openjdk.java.net/browse/JDK-8225239 Webrev: http://cr.openjdk.java.net/~redestad/8225239/open.01/ - adds a package-private method isBoundInetAddress that shortcuts some of the paths taken by existing methods when all we want to know is whether an IP address is bound to some interface on this system. This allows us to iterate over only IPv4 interfaces if the sought after IP address is an IPv4 address and vice versa for IPv6 addresses. This means a small (1.1x) to large (5x) speedup on a variety of systems, depending on how many IPv4 vs IPv6 interfaces exist on the system.. - refactors how the Windows-specific GetIPAddrTable API is used: current implementation looks up the global IP address table over and over, once per bound address, which means time spent by operations like NetworkInterface.getByInetAddress grows at least quadratically with the number of interfaces on a system. Refactoring these methods so that GetIPAddrTable is called once means a dramatic improvement on some of the API calls, e.g getByInetAddress cost drops from ~100ms to ~10ms on one of our larger test systems. - Added some additional tests and a microbenchmark. Testing: tier1-4 (some still ongoing, but all green so far) Thanks! /Claes
Re: RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler
On 2019-05-02 15:20, Seán Coffey wrote: Hi Claes, Yes - looks like a fine suggestion. http://cr.openjdk.java.net/~coffeys/webrev.8217364.03/webrev/ Looks good to me.. ;-) /Claes
Re: RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler
Hi Seán,
wouldn't it be more straightforward then to keep the logic intact and
skip the custom factory invocation in both cases if the protocol is
non-overrideable?
I.e., something like this:
diff -r 290283590646 src/java.base/share/classes/java/net/URL.java
--- a/src/java.base/share/classes/java/net/URL.java Tue Apr 30 23:47:00
2019 +0200
+++ b/src/java.base/share/classes/java/net/URL.java Thu May 02 14:10:57
2019 +0200
@@ -1403,8 +1403,9 @@
URLStreamHandlerFactory fac;
boolean checkedWithFactory = false;
+boolean overrideableProtocol = isOverrideable(protocol);
-if (isOverrideable(protocol) && jdk.internal.misc.VM.isBooted()) {
+if (overrideableProtocol && jdk.internal.misc.VM.isBooted()) {
// Use the factory (if any). Volatile read makes
// URLStreamHandlerFactory appear fully initialized to
current thread.
fac = factory;
@@ -1440,7 +1441,7 @@
// Check with factory if another thread set a
// factory since our last check
-if (!checkedWithFactory && (fac = factory) != null) {
+if (overrideableProtocol && !checkedWithFactory && (fac =
factory) != null) {
handler2 = fac.createURLStreamHandler(protocol);
}
Thanks!
/Claes
On 2019-05-02 12:33, Seán Coffey wrote:
with webrev: https://cr.openjdk.java.net/~coffeys/webrev.8217364.02/webrev/
regards,
Sean.
On 02/05/2019 11:00, Seán Coffey wrote:
Been thinking a bit more about this one.
Given that only initialization code will traverse through the "if
(!isOverrideable(protocol))" check, I think we can add synchronization
to eliminate any timing scenarios where the handlers Hashtable gets
populated via another thread after we make the first handers.get call
in getURLStreamHandler(String protocol)
regards,
Sean.
On 30/04/2019 18:19, Seán Coffey wrote:
Looking to correct an issue that arose during the JDK-8213942 fix.
https://bugs.openjdk.java.net/browse/JDK-8217364
https://cr.openjdk.java.net/~coffeys/webrev.8217364/webrev/
regards,
Sean.
Re: RFR: 8215990: Avoid using reflection to create common default URLStreamHandlers
Hi, On 2019-01-02 11:04, Alan Bateman wrote: On 02/01/2019 09:02, Claes Redestad wrote: Hi, during bootstrap we load and use at least two of the jrt, file and jar URLStreamHandlers via URL$DefaultFactory. Avoiding use of reflection to instantiate these slightly speed up bootstrap by reducing number of classes loaded and doing fewer relatively expensive reflective operations: http://cr.openjdk.java.net/~redestad/8215990/jdk.00/ This looks okay to me and will continue to work when the protocol is not in lower case (URL protocols/schemes are compared with regard to case). thanks! (The protocol input string should already be lower-case at this point) While we're in the area then it would be nice to eliminate the use of Class::newInstance too - if each of the built-in protocol handlers is public with a public no-arg constructor then getConstructor().newInstance() should do it. To be perfectly compatible I think .getDeclaredConstructor().newInstance() should be used: http://cr.openjdk.java.net/~redestad/8215990/jdk.01 A bigger job of work in this area is to replace the use of Hashtable (handlers field). That's something for a bigger overhaul of course. We shy-ed away from doing this in JDK 9 due to OSGi implementations hacking the field directly. I'm in favor of any effort to get rid of Hashtable usage, anywhere. :-) /Claes
RFR: 8215990: Avoid using reflection to create common default URLStreamHandlers
Hi, during bootstrap we load and use at least two of the jrt, file and jar URLStreamHandlers via URL$DefaultFactory. Avoiding use of reflection to instantiate these slightly speed up bootstrap by reducing number of classes loaded and doing fewer relatively expensive reflective operations: http://cr.openjdk.java.net/~redestad/8215990/jdk.00/ Thanks! /Claes
Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base
On 2018-12-13 14:06, Daniel Fuchs wrote: Looks good Claes! Thanks! I eyeballed the rest of the patch and found nothing wrong - but with a patch this size it would be easy to miss something. Yes, I've gone through it a couple of times now to be sure. Were you able to measure any improvement after patching? There's a tiny reduction in bytecode executed in initPhase1, but on any real measures any improvement is in the noise. /Claes best regards, -- daniel
Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base
On 2018-12-12 17:54, Daniel Fuchs wrote:
Hi Claes,
It might read even better if things like:
+ resultString = !specarg.isEmpty() ? specarg.intern(): opt;
were changed into:
+ resultString = specarg.isEmpty() ? opt : specarg.intern();
best regards,
I only found this pattern in this file, so incremental
diff will look like the below. I will update in place due hugeness of
webrev.
Thanks!
/Claes
diff -r 732b03e40349
src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java
Wed Dec 12 17:41:46 2018 +0100
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java
Wed Dec 12 18:03:57 2018 +0100
@@ -641,10 +641,10 @@
String specarg = spec.substring(sidx);
switch (specop) {
case '.': // terminate the option sequence
-resultString = !specarg.isEmpty() ?
specarg.intern(): opt;
+resultString = specarg.isEmpty() ? opt :
specarg.intern();
break doArgs;
case '?': // abort the option sequence
-resultString = !specarg.isEmpty() ?
specarg.intern(): arg;
+resultString = specarg.isEmpty() ? arg :
specarg.intern();
isError = true;
break eachSpec;
case '@': // change the effective opt name
@@ -655,7 +655,7 @@
val = "";
break;
case '!': // negation option
-String negopt = !specarg.isEmpty() ?
specarg.intern(): opt;
+String negopt = specarg.isEmpty() ? opt :
specarg.intern();
properties.remove(negopt);
properties.put(negopt, null); // leave
placeholder
didAction = true;
Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base
On 2018-12-12 17:56, Alan Bateman wrote: In Checks.java, the parameter change from CharSequence to String means that "cs" needs to be renamed. Changed to 'str' /Claes
RFR: 8215281: Use String.isEmpty() where applicable in java.base
Hi, please review this patch to use String.isEmpty when applicable: Webrev: http://cr.openjdk.java.net/~redestad/8215281/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8215281 Why? - It reads better :-) - Better startup/warmup due fewer method invocations - Better peak performance: String.isEmpty is ~1.2x faster than String.length Most changes are simple search and replace, but I took the liberty to clean out some dead/pointless uses and improve formatting in places due the shorter expression length. Instances of "".equals(string) were only changed when it was immediately obvious that string is not null, e.g., due a preceding null check. Thanks! /Claes
Re: 8199329: Remove code that attempts to read bytes after connection reset reported
Looks good to me. /Claes On 2018-03-14 15:30, Alan Bateman wrote: Classic networking has some curious code to deal with connection resets. I needed to dig into ancient history to find the issues and original motivations. When a connection reset is reported (usually ECONNRESET) then further attempts to read from the socket will typically return -1 (EOF). Classic networking papers over this so that further attempts with the socket APIs to read bytes will continue to throw SocketException "connection reset". Furthermore, it allows for cases where the platform can read bytes from the socket buffer after ECONNRESET has been reported. None of the main stream platforms do this and it's hard to imagine anything depending on such unpredictable behavior. I'm running into this odd code as part of cleanup to the read/write code paths and reducing them down to a single blocking call. I would like to remove the legacy/undocumented behavior that attempts to read beyond the connection reset. The code to consistently throw IOException once ECONNRESET is returned is retained as it's possible that code relies on this. The proposed changes are here: http://cr.openjdk.java.net/~alanb/8199329/webrev/ All existing tests pass with these changes. -Alan
Re: [11] RFR: 8197849: Misc improvements to URL parsing
Thanks for reviewing, Roger /Claes On 2018-02-14 17:50, Roger Riggs wrote: Hi Claes, Looks fine, Roger On 2/14/2018 11:36 AM, Claes Redestad wrote: Right, I expanded the range as much as possible given the constraint provided by L_ENCODED minus '/'. I will think of a better comment to this effect. /Claes Daniel Fuchs <daniel.fu...@oracle.com> skrev: (14 februari 2018 17:25:20 CET) Hi Claes, Your proposed changes to ParseUtil look reasonable to me, though I had to carefully compare the characters in the range (c >= '&' && c <= ':') with the L_ENCODED / H_ENCODED masks to convince myself that there was no behavior change to ParseUtil::firstEncodeIndex. I wonder whether this would deserve some additional comment - though I'm not sure how it could be formulated. Given the sensitivity of the impacted code maybe it would be prudent to wait for a second review before pushing. best regards, -- daniel On 14/02/2018 15:30, Claes Redestad wrote: Hi, as a means to improve startup in some applications, please review this set of small improvements to improve both interpreted and compiled performance of creating and handling certain jar URLs. Some of the changes in sun.net.www.ParseUtil::encodePath have a small, positive effect when dealing with other types of path resources. Bug: https://bugs.openjdk.java.net/browse/JDK-8197849 Webrev: http://cr.openjdk.java.net/~redestad/8197849/jdk.00/ <http://cr.openjdk.java.net/%7Eredestad/8197849/jdk.00/> This shaves off a percent or so of the total bytecode execution in a few of our startup tests: - ParseUtil::encodePath cost is reduced ~20% during startup, averaging ~10-15% faster for typical inputs after JIT. Weird examples like paths only consisting of slashes and dots can be seen to take a small hit due to not getting special treatment. - ParseUtil::canonizeString cost on startup reduced by 50% (~15% improvement after JIT) for typical inputs by adding a test to return directly if there's no need to "canonize" the string (which is typically always the case for well-formed jar files). I added a sanity test to ensure I didn't accidentally change semantics of cases that would lead to canonicalization. - Removed a couple of unnecessary allocation in sun.net.www.protocol.jar.Handler. Maybe there are some good reasons not to make ParseUtil a final utility class with only static methods and a private constructor, though... Thanks! /Claes -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [11] RFR: 8197849: Misc improvements to URL parsing
Right, I expanded the range as much as possible given the constraint provided by L_ENCODED minus '/'. I will think of a better comment to this effect. /Claes Daniel Fuchs <daniel.fu...@oracle.com> skrev: (14 februari 2018 17:25:20 CET) >Hi Claes, > >Your proposed changes to ParseUtil look reasonable >to me, though I had to carefully compare the characters >in the range (c >= '&' && c <= ':') with the >L_ENCODED / H_ENCODED masks to convince myself >that there was no behavior change to >ParseUtil::firstEncodeIndex. > >I wonder whether this would deserve some additional >comment - though I'm not sure how it could be formulated. > >Given the sensitivity of the impacted code maybe it would >be prudent to wait for a second review before pushing. > >best regards, > >-- daniel > >On 14/02/2018 15:30, Claes Redestad wrote: >> Hi, >> >> as a means to improve startup in some applications, please review >this >> set of small improvements to improve both interpreted and compiled >> performance of creating and handling certain jar URLs. Some of the >> changes in sun.net.www.ParseUtil::encodePath have a small, positive >> effect when dealing with other types of path resources. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8197849 >> >> Webrev: http://cr.openjdk.java.net/~redestad/8197849/jdk.00/ >> >> This shaves off a percent or so of the total bytecode execution in a >few >> of our startup tests: >> >> - ParseUtil::encodePath cost is reduced ~20% during startup, >averaging >> ~10-15% faster for typical inputs after JIT. Weird examples like >paths >> only consisting of slashes and dots can be seen to take a small hit >due >> to not getting special treatment. >> >> - ParseUtil::canonizeString cost on startup reduced by 50% (~15% >> improvement after JIT) for typical inputs by adding a test to return >> directly if there's no need to "canonize" the string (which is >typically >> always the case for well-formed jar files). I added a sanity test to >> ensure I didn't accidentally change semantics of cases that would >lead >> to canonicalization. >> >> - Removed a couple of unnecessary allocation in >> sun.net.www.protocol.jar.Handler. Maybe there are some good reasons >not >> to make ParseUtil a final utility class with only static methods and >a >> private constructor, though... >> >> Thanks! >> >> /Claes -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
[11] RFR: 8197849: Misc improvements to URL parsing
Hi, as a means to improve startup in some applications, please review this set of small improvements to improve both interpreted and compiled performance of creating and handling certain jar URLs. Some of the changes in sun.net.www.ParseUtil::encodePath have a small, positive effect when dealing with other types of path resources. Bug: https://bugs.openjdk.java.net/browse/JDK-8197849 Webrev: http://cr.openjdk.java.net/~redestad/8197849/jdk.00/ This shaves off a percent or so of the total bytecode execution in a few of our startup tests: - ParseUtil::encodePath cost is reduced ~20% during startup, averaging ~10-15% faster for typical inputs after JIT. Weird examples like paths only consisting of slashes and dots can be seen to take a small hit due to not getting special treatment. - ParseUtil::canonizeString cost on startup reduced by 50% (~15% improvement after JIT) for typical inputs by adding a test to return directly if there's no need to "canonize" the string (which is typically always the case for well-formed jar files). I added a sanity test to ensure I didn't accidentally change semantics of cases that would lead to canonicalization. - Removed a couple of unnecessary allocation in sun.net.www.protocol.jar.Handler. Maybe there are some good reasons not to make ParseUtil a final utility class with only static methods and a private constructor, though... Thanks! /Claes
Re: [10] RFR: 8186930: Constant fold URI constants
On 2017-08-30 18:18, Chris Hegarty wrote: Thanks for doing this Claes, this latest version looks good to me. Thanks Chris! /Claes
Re: [10] RFR: 8186930: Constant fold URI constants
Hi Daniel, I'm not sure: I have no love for whitebox tests in general, and especially not for code where doing exhaustive black box tests against a public API is possible and perhaps even straightforward (the test coverage seems adequate currently, but can of course always be improved). /Claes On 2017-08-30 18:16, Daniel Fuchs wrote: Hi Claes, Maybe it could be interesting to leave the methods in a static inner class which is not referenced (except by whitebox tests) and have this inner class expose a static test() method that would test that the static fields in URI have the expected value? This is just a suggestion, and it could be done in a followup RFE... best regards, -- daniel On 31/08/2017 16:26, Claes Redestad wrote: On 2017-08-30 17:17, Alan Bateman wrote: I think it could be useful as someone reading the code isn't going to immediately know to jump to URI. Done: http://cr.openjdk.java.net/~redestad/8186930/jdk.01/ /Claes
Re: [10] RFR: 8186930: Constant fold URI constants
On 2017-08-30 17:17, Alan Bateman wrote: I think it could be useful as someone reading the code isn't going to immediately know to jump to URI. Done: http://cr.openjdk.java.net/~redestad/8186930/jdk.01/ /Claes
Re: [10] RFR: 8186930: Constant fold URI constants
On 2017-08-30 16:10, Alan Bateman wrote: On 29/08/2017 21:26, Claes Redestad wrote: Webrev: http://cr.openjdk.java.net/~redestad/8186930/jdk.00/ This looks good to me. Thanks! I just wonder if ParseUtil should keep the lowMask/highMask methods in comments so that further maintainers can satisfy themselves that the values are correct. I thought leaving a comment in ParseUtil that they can be still found in URI was sufficient (cuts back on the redundancy), but I can keep them in comments in both places if you think that's preferable. /Claes
[10] RFR: 8186930: Constant fold URI constants
Hi, please review this patch to pre-calculate constants in java.net.URI and sun.net.www.ParseUtil, removing work from runtime (reduces bytecodes executed in the interpreter on bootstrap by ~15K). This also removes use of BitSet from ParseUtil, which apart from being a startup improvement also yields a small throughput improvement (~5%). Webrev: http://cr.openjdk.java.net/~redestad/8186930/jdk.00/ Thanks! /Claes
Re: RFR 8179905: Remove the use of gettimeofday in Networking code
Hi, yes, when we do int -> jlong conversion before multiplication is fine, but there are some methods, such as NET_Timeout that takes long timeout that seemed in danger. After offline discussion it seems these are always called with what's essentially an int thus we should be safe, but as a follow-up I suggest narrowing to int or jint in such method signatures just to make this clear. Thanks! /Claes On 2017-05-10 08:28, Vyom Tewari wrote: Hi Claes, thanks for review, timeout is "int" so even if you set max(2147483647) value that int data type can hold, there will not be any overflow. If you try to set very large number like "0x7fff" to int data type you will get compile time error(integer number too large: 7fff). Thanks, Vyom On Tuesday 09 May 2017 11:20 PM, Claes Redestad wrote: Hi, doesn't this need to consider numerical overflows, e.g., what happens if someone sets a timeout value near 0x7fffL before and after this change? /Claes On 2017-05-09 11:55, Vyom Tewari wrote: Hi , Please review the code change for below issue. Webrev : http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html BugId : https://bugs.openjdk.java.net/browse/JDK-8179905 This issue is duplicate of "JDK-8165437", where pushed code change failed on 32 bit platforms so we revert back code changes as part of "JDK-8179602". Please find below is the webrev that was pushed as part of "JDK-8165437". Webrev : http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html Thanks, Vyom
Re: RFR 8179905: Remove the use of gettimeofday in Networking code
Hi, doesn't this need to consider numerical overflows, e.g., what happens if someone sets a timeout value near 0x7fffL before and after this change? /Claes On 2017-05-09 11:55, Vyom Tewari wrote: Hi , Please review the code change for below issue. Webrev : http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html BugId : https://bugs.openjdk.java.net/browse/JDK-8179905 This issue is duplicate of "JDK-8165437", where pushed code change failed on 32 bit platforms so we revert back code changes as part of "JDK-8179602". Please find below is the webrev that was pushed as part of "JDK-8165437". Webrev : http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html Thanks, Vyom
RFR: 8170785: Excessive allocation in ParseUtil.encodePath
Hi, some users reports high allocation rates in ParseUtil.encodePath, regardless of whether paths actually need to be encoded or not. Since this is a commonly used utility it makes sense. Webrev: http://cr.openjdk.java.net/~redestad/8170785/webrev.01 Bug: https://bugs.openjdk.java.net/browse/JDK-8170785 This patch provides a semantically neutral fast-path for cases when the path does not need to be encoded (up to 5x speedup), reduces allocation when the string has a prefix that does not need to be encoded (1-2x speedup) and no regression when using a separator that's not '/' or the first char needs encoding. Interpreted performance is not affected much either: small positive when no encoding is needed, neutral or negligible regression otherwise. Thanks! /Claes
Re: RFR: 8159745: Remove java.net.Parts
On 2016-06-17 16:26, Chris Hegarty wrote: On 17/06/16 15:05, Claes Redestad wrote: On 2016-06-17 16:04, Alan Bateman wrote: On 17/06/2016 14:43, Claes Redestad wrote: Hi, URL.java defines a package-private class Parts, which can be trivially inlined in the one place where it's currently used. Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8159745 I suspect that class had other usages in old releases. Your patch looks okay, maybe change `ind` to `index` or something that doesn't look like `int` :-) Thanks, I'll update ind before push. :-) +1 Thanks Claes. Thanks for the reviews, pushed! /Claes
Re: RFR: 8159745: Remove java.net.Parts
On 2016-06-17 16:04, Alan Bateman wrote: On 17/06/2016 14:43, Claes Redestad wrote: Hi, URL.java defines a package-private class Parts, which can be trivially inlined in the one place where it's currently used. Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8159745 I suspect that class had other usages in old releases. Your patch looks okay, maybe change `ind` to `index` or something that doesn't look like `int` :-) Thanks, I'll update ind before push. :-) /Claes
RFR: 8159745: Remove java.net.Parts
Hi, URL.java defines a package-private class Parts, which can be trivially inlined in the one place where it's currently used. Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8159745 Thanks! /Claes
Re: RFR: 8154454: Fix compilation issue in PlainSocketImpl
Thanks for the quick review Chris,
I'll push as soon as JPRT runs look green.
/Claes
On 04/18/2016 04:45 PM, Chris Hegarty wrote:
Looks ok Claes.
-Chris.
On 18/04/16 15:38, Claes Redestad wrote:
Hi,
a small omission in JDK-8154238 cause Windows builds to fail. Sorry
about that, see patch to fix this below (I was 100% certain I had run
this
through JPRT last week)
Bug: https://bugs.openjdk.java.net/browse/JDK-8154454
Thanks!
/Claes
diff -r 3459ee432728
src/java.base/windows/classes/java/net/PlainSocketImpl.java
--- a/src/java.base/windows/classes/java/net/PlainSocketImpl.java Mon
Apr 18 14:01:03 2016 +0200
+++ b/src/java.base/windows/classes/java/net/PlainSocketImpl.java Mon
Apr 18 16:35:03 2016 +0200
@@ -80,7 +80,7 @@
* Constructs an instance with the given file descriptor.
*/
PlainSocketImpl(FileDescriptor fd) {
-if (useDualStackImpl) {
+if (!preferIPv4Stack) {
impl = new DualStackPlainSocketImpl(fd, exclusiveBind);
} else {
impl = new TwoStacksPlainSocketImpl(fd, exclusiveBind);
RFR: 8154454: Fix compilation issue in PlainSocketImpl
Hi,
a small omission in JDK-8154238 cause Windows builds to fail. Sorry
about that, see patch to fix this below (I was 100% certain I had run this
through JPRT last week)
Bug: https://bugs.openjdk.java.net/browse/JDK-8154454
Thanks!
/Claes
diff -r 3459ee432728
src/java.base/windows/classes/java/net/PlainSocketImpl.java
--- a/src/java.base/windows/classes/java/net/PlainSocketImpl.java Mon
Apr 18 14:01:03 2016 +0200
+++ b/src/java.base/windows/classes/java/net/PlainSocketImpl.java Mon
Apr 18 16:35:03 2016 +0200
@@ -80,7 +80,7 @@
* Constructs an instance with the given file descriptor.
*/
PlainSocketImpl(FileDescriptor fd) {
-if (useDualStackImpl) {
+if (!preferIPv4Stack) {
impl = new DualStackPlainSocketImpl(fd, exclusiveBind);
} else {
impl = new TwoStacksPlainSocketImpl(fd, exclusiveBind);
Re: RFR: 8154238: Drop code to support Windows XP in windows socket impl
On 04/18/2016 12:01 PM, Alan Bateman wrote: On 18/04/2016 10:45, Chris Hegarty wrote: The changes look fine. Maybe the bug description should be updated a before pushing, it looks like it affects only legacy networking code and not NIO. Maybe "... and async channels" ? The changes to the NIO code look okay but probably should be split into their own issue for tracking purposes. Ah, now I get what Chris meant. Sure. I'll break those off to a separate bug. /Claes
RFR: 8154238: Drop code to support Windows XP in windows socket impl
Hi, more code in the Windows socket implementation that can be dropped Bug: https://bugs.openjdk.java.net/browse/JDK-8154238 Webrev: http://cr.openjdk.java.net/~redestad/8154238/webrev.01/ Thanks! /Claes
Re: RFR: 8154185: Drop code to support Windows XP in DefaultDatagramSocketImplFactory
On 04/13/2016 11:43 PM, Chris Hegarty wrote: On 13 Apr 2016, at 22:31, Claes Redestad <claes.redes...@oracle.com> wrote: Hi, since we're dropping support for Windows XP in JDK 9, we can simplify initialization of DefaultDatagramSocketImplFactory. Bug: https://bugs.openjdk.java.net/browse/JDK-8154185 Webrev: http://cr.openjdk.java.net/~redestad/8154185/webrev.01/ This looks good to me Claes. -Chris. Thanks, Chris! /Claes
RFR: 8154185: Drop code to support Windows XP in DefaultDatagramSocketImplFactory
Hi, since we're dropping support for Windows XP in JDK 9, we can simplify initialization of DefaultDatagramSocketImplFactory. Bug: https://bugs.openjdk.java.net/browse/JDK-8154185 Webrev: http://cr.openjdk.java.net/~redestad/8154185/webrev.01/ Thanks! /Claes
Re: RFR: 8151299 Http client SelectorManager overwriting read and write events
On 2016-03-08 14:42, Michael McMahon wrote:
Iterator-based removal would work, or building a new list to replace
pending with might be more efficient. The synchronization scheme
seems a bit flaky as well?
The new code is always called from the same thread. So, I don't see an
issue?
Right, but it seems the SelectorAttachment can escape and be observed by
other threads that may have access to the SelectableChannel. I don't
know if this could become a problem somewhere, but it makes me a bit
uneasy. Maybe the attachment should be an immutable key to a map of some
tracking object held by and managed by the SelectorManager?
Also, with the assumption that the SelectorManager is running in a
distinct thread, couldn't this be checked at the start of run() and then
avoid synchronized(this) in the loop?
Another oddity in the surrounding code is that
System.currentTimeMillis() is always called, but only used when
selector.select(timeval) == 0 - could this be moved into the if-block?
266 long now = System.currentTimeMillis();
267 int n = selector.select(timeval);
268 if (n == 0) {
269 signalTimeouts(now);
270 continue;
271 }
Thanks.
/Claes
Re: RFR: 8148626: URI.toURL needs to use protocol Handler to parse file URIs
Joe, Chris, thanks for reviewing. Pushed. /Claes On 2016-01-31 17:13, Chris Hegarty wrote: On 31 Jan 2016, at 00:58, Claes Redestad <claes.redes...@oracle.com> wrote: Hi, On 2016-01-30 19:35, joe darcy wrote: Hello, The change looks okay in that the new code is limited to the jrt protocol. I assume the failing test java/net/URL/B5086147.java passes again with this change. I had to go through some hoops to find out how to ensure this test actually gets picked up by the remote-testing incantation I've been using so far, but have verified the test has been run and passes on the affected platforms. If a fix for this issue is not pushed soon, I want to have the failing test problem listed (http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038459.html). I'm ready push this, but might be good to get a second opinion from net-dev to ensure they aren't caught unaware of this follow-up fix. This looks ok Claes. -Chris. Thanks! /Claes Thanks, -Joe On 1/30/2016 6:30 AM, Claes Redestad wrote: Hi, it turns out trying to optimize URI to URL conversion for file URLs don't work out very well due to special treatment of backward slashes. Instead of trying to deal with such file URLs, this patch partially backs out 8147462 and only leave the optimization in place for the jrt protocol. This still helps reducing footprint in jigsaw while keeping things performance neutral for other kinds of URLs. Bug: https://bugs.openjdk.java.net/browse/JDK-8148626 Webrev: http://cr.openjdk.java.net/~redestad/8148626/webrev.01/ /Claes
Re: RFR: 8148626: URI.toURL needs to use protocol Handler to parse file URIs
Hi, On 2016-01-30 19:35, joe darcy wrote: Hello, The change looks okay in that the new code is limited to the jrt protocol. I assume the failing test java/net/URL/B5086147.java passes again with this change. I had to go through some hoops to find out how to ensure this test actually gets picked up by the remote-testing incantation I've been using so far, but have verified the test has been run and passes on the affected platforms. If a fix for this issue is not pushed soon, I want to have the failing test problem listed (http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/038459.html). I'm ready push this, but might be good to get a second opinion from net-dev to ensure they aren't caught unaware of this follow-up fix. Thanks! /Claes Thanks, -Joe On 1/30/2016 6:30 AM, Claes Redestad wrote: Hi, it turns out trying to optimize URI to URL conversion for file URLs don't work out very well due to special treatment of backward slashes. Instead of trying to deal with such file URLs, this patch partially backs out 8147462 and only leave the optimization in place for the jrt protocol. This still helps reducing footprint in jigsaw while keeping things performance neutral for other kinds of URLs. Bug: https://bugs.openjdk.java.net/browse/JDK-8148626 Webrev: http://cr.openjdk.java.net/~redestad/8148626/webrev.01/ /Claes
Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs
On 2016-01-28 20:56, Alan Bateman wrote: This looks okay. I think it would be good to put a javadoc comment on this method, also maybe move it to below all the constructors rather than in the middle. -Alan. Sure, as this is package-private API a short description and reference to the URI::toURL method whose specification we adhere to should suffice: http://cr.openjdk.java.net/~redestad/8147462/webrev.06/ Thanks! /Claes
Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs
On 2016-01-28 16:52, Chris Hegarty wrote: ... This looks fine Claes. Thanks, Chris! Maybe just a comment on the fact that character based comparison is being used for perf reasons. In an older version of the webrev, there was an updated to an existing test, test/java/net/URI/URItoURLTest.java. I assume this was just mistakenly omitted from the latest version? Fixed test and added a comment: http://cr.openjdk.java.net/~redestad/8147462/webrev.05/ Thanks! /Claes
Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs
On 2016-01-18 17:20, Claes Redestad wrote: The ability for URLStreamHandler implementations to override the parseURL method seem to prevent this improvement unless we only do this for a subset of known, well-behaved URLStreamHandlers, which likely defeat the optimization by adding complexity. Right, the fast path can only be safely used for the non-overrideable handlers (jrt and file), but turns out we can make that work without penalizing other cases: http://cr.openjdk.java.net/~redestad/8147462/webrev.04/ Relevant micros[1] show that this brings a benefit to file/jrt, even when mixed with slow path URIs, while micros only hitting slow path (newHttpURIToURL, opaqueURIToURL) doesn't regress: Before: Benchmark Mode CntScoreError Units URIBench.mixedURIToURLavgt 30 463.748 ± 14.445 ns/op URIBench.newHttpURIToURL avgt 30 441.497 ± 20.173 ns/op URIBench.newURIToURL avgt 30 227.106 ± 9.055 ns/op URIBench.opaqueURIToURL avgt 30 320.904 ± 13.232 ns/op Patched: Benchmark Mode CntScoreError Units URIBench.mixedURIToURLavgt 30 441.773 ± 16.530 ns/op URIBench.newHttpURIToURL avgt 30 433.946 ± 18.569 ns/op URIBench.newURIToURL avgt 30 147.379 ± 6.349 ns/op URIBench.opaqueURIToURL avgt 30 316.632 ± 12.940 ns/op /Claes [1] http://cr.openjdk.java.net/~redestad/8147462/URIBench.java
Re: RFR: 8147962: URL should handle lower-casing of protocol locale-independently
On 2016-01-22 12:29, Alan Bateman wrote: On 21/01/2016 17:27, Claes Redestad wrote: Hi, using String.toLowerCase() in URL allows some strings to lower-case to a string that'd be invalid, e.g. "FILE".toLowerCase() -> "f\u0131le" in turkish locales. This patch suggests using the locale-independent String.toLowerCase(Locale.ENGLISH) instead, since only a-z and A-Z are legal alphabeticals according to spec. Webrev: http://cr.openjdk.java.net/~redestad/8147962/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8147962 The change to URL looks okay but be warned that URL has bitten the fingers of many that have dared to touch it. So we'll need to keep an eye out for any side effects/bug reports. I've been questioning my sanity for some time now. For the test then maybe it could iterate over all locales rather than just test with tr. Also good for the test to run in othervm mode, I can't recall off hand if jtreg resets the locale when in agent VM mode (the default in our testing these days). Done: http://cr.openjdk.java.net/~redestad/8147962/webrev.02/ Going over available locales will make the test pass on environments that don't happen to have the "tr" locale (and verified this catches the issue). Also switched to Locale.ROOT as Naoto suggested. Thanks! /Claes
Re: RFR: 8147962: URL should handle lower-casing of protocol locale-independently
On 2016-01-22 14:38, Alan Bateman wrote: On 22/01/2016 12:29, Claes Redestad wrote: : http://cr.openjdk.java.net/~redestad/8147962/webrev.02/ This looks good to me. -Alan Thanks! /Claes
RFR: 8147962: URL should handle lower-casing of protocol locale-independently
Hi, using String.toLowerCase() in URL allows some strings to lower-case to a string that'd be invalid, e.g. "FILE".toLowerCase() -> "f\u0131le" in turkish locales. This patch suggests using the locale-independent String.toLowerCase(Locale.ENGLISH) instead, since only a-z and A-Z are legal alphabeticals according to spec. Webrev: http://cr.openjdk.java.net/~redestad/8147962/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8147962 Thanks! Claes
Re: RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs
On 2016-01-18 15:34, Chris Hegarty wrote:
On 18/01/16 14:09, Alan Bateman wrote:
On 17/01/2016 15:30, Claes Redestad wrote:
Hi,
please review this patch which might make URI.toURL more efficient
Webrev: http://cr.openjdk.java.net/~redestad/8147462
Bug: https://bugs.openjdk.java.net/browse/JDK-8147462
This patch exploits the fact that URI will apply the same validation
to the URI/URL specification for any valid non-opaque URL, thus it's
safe to use the component-based URL constructor. Also, URIs
representing malformed URLs will throw an exception as specified both
before and after. A number of simple test cases to capture and
document this was added to the existing java/net/URI/URItoURL jtreg
test.
I think the change to URI looks okay but it would be good to include a
brief comment as otherwise it will be difficult for future maintainers
to understand.
+1. For a long time we have been suggesting that anyone requiring
a URL should retrieve it from URI::toURL, so it is good to have a
fast(er) common path.
The handling of a null/empty host is a little esoteric, so a comment
would be good.
How about this?
http://cr.openjdk.java.net/~redestad/8147462/webrev.02
I do have a little discomfort about this change, since the toURL spec
specifically says it is "equivalent to evaluating the expression new
URL(this.toString())". I wonder if your tests should cover this too,
i.e. the resulting URLs from toURL and URL(this.toString()) are equal?
The test already does this. I've just added cases that exercise some
interesting cases like URIs containing /./ and quoted characters.
I restructured the exceptional tests slightly to differentiate the cases
where both toURL and new URL throw MalformedURLException from cases
where toURL throws IAE. It makes the test slightly more verbose but
arguably a lot simpler.
However: the javadoc spec for URL(String) seems incomplete, and I might
have stumbled into a trap...
* @exception MalformedURLException if no protocol is specified,
or an
* unknown protocol is found, or {@code spec} is
{@code null}.
Since this method delegates parsing to the URLStreamHandler associated
with the protocol it might be overriden to throw exceptions (that will
be wrapped in MalformedURLException) for a variety of reasons.
The ability for URLStreamHandler implementations to override the
parseURL method seem to prevent this improvement unless we only do this
for a subset of known, well-behaved URLStreamHandlers, which likely
defeat the optimization by adding complexity.
Thanks!
/Claes
RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs
Hi, please review this patch which might make URI.toURL more efficient Webrev: http://cr.openjdk.java.net/~redestad/8147462 Bug: https://bugs.openjdk.java.net/browse/JDK-8147462 This patch exploits the fact that URI will apply the same validation to the URI/URL specification for any valid non-opaque URL, thus it's safe to use the component-based URL constructor. Also, URIs representing malformed URLs will throw an exception as specified both before and after. A number of simple test cases to capture and document this was added to the existing java/net/URI/URItoURL jtreg test. Microbenchmarks covering various URIs vary from neutral (for opaque) to 1.5x (for the simplest URIs without query and fragment components), while also bringing in a small footprint improvement in jake builds. While toURL() could arguably construct the URL file as path[?query][#fragment], the implementation I tried for this caused regressions in micros due to hitting C2 inlining limit. Thanks! /Claes
Re: RFR: 8146686: Create the schemeSpecificPart for non-opaque URIs lazily
On 2016-01-10 10:39, Chris Hegarty wrote: On 8 Jan 2016, at 14:41, Claes Redestad <claes.redes...@oracle.com> wrote: Hi, please review this patch to lazily create schemeSpecificPart for non-opaque URIs. This change accounts for some improvement in footprint characteristics and a 10-20% improvement on URI creation microbenchmarks, while not regressing notably on a targetted microbenchmark which explicitly generate and retrieve the schemeSpecificPart. Bug: https://bugs.openjdk.java.net/browse/JDK-8146686 Webrev: http://cr.openjdk.java.net/~redestad/8146686/webrev.01/ Looks ok to me Claes. -Chris. Thanks for reviews, Chris and Alan! /Claes
RFR: 8146686: Create the schemeSpecificPart for non-opaque URIs lazily
Hi, please review this patch to lazily create schemeSpecificPart for non-opaque URIs. This change accounts for some improvement in footprint characteristics and a 10-20% improvement on URI creation microbenchmarks, while not regressing notably on a targetted microbenchmark which explicitly generate and retrieve the schemeSpecificPart. Bug: https://bugs.openjdk.java.net/browse/JDK-8146686 Webrev: http://cr.openjdk.java.net/~redestad/8146686/webrev.01/ Thanks! /Claes
Re: RFR: 8146686: Create the schemeSpecificPart for non-opaque URIs lazily
On 2016-01-08 16:34, Alan Bateman wrote: On 08/01/2016 14:41, Claes Redestad wrote: Hi, please review this patch to lazily create schemeSpecificPart for non-opaque URIs. This change accounts for some improvement in footprint characteristics and a 10-20% improvement on URI creation microbenchmarks, while not regressing notably on a targetted microbenchmark which explicitly generate and retrieve the schemeSpecificPart. Bug: https://bugs.openjdk.java.net/browse/JDK-8146686 Webrev: http://cr.openjdk.java.net/~redestad/8146686/webrev.01/ This looks okay to me. The only thing that I wonder about is the original code returning null when getRawSchemeSpecificPart is specified to never return null. Yes, I think that can't ever happen for any valid URI since during parse either schemeSpecificPart or path will be set to a non-null value. I propagated this confusion in one of the recent patches to be semantically identical with the legacy implementation. /Claes -Alan.
Re: RFR: 8146526: Improve java.net.URI$Parser startup characteristics
On 2016-01-06 08:43, Alan Bateman wrote: On 05/01/2016 22:47, Claes Redestad wrote: Hi, please review this patch to cleanup URI$Parser to help URI construction when run with the interpreter, mostly by inlining wrapping methods: Bug: https://bugs.openjdk.java.net/browse/JDK-8146526 Webrev: http://cr.openjdk.java.net/~redestad/8146526/webrev.01 This looks good to me. Thanks for looking at this, Alan and Chris! /Claes
RFR: 8146526: Improve java.net.URI$Parser startup characteristics
Hi, please review this patch to cleanup URI$Parser to help URI construction when run with the interpreter, mostly by inlining wrapping methods: Bug: https://bugs.openjdk.java.net/browse/JDK-8146526 Webrev: http://cr.openjdk.java.net/~redestad/8146526/webrev.01 This is motivated by Jigsaw where URIs might be created unconditionally during startup, and this trivial patch is extracted from an experiment to address observed inefficiencies in java.net.URI[1]. Around half the improvement detailed in [1] can be attributed to this patch, while it does not impact compiled code performance. Thanks! /Claes [1]http://cr.openjdk.java.net/~redestad/scratch/URIParserBench.java
Re: RFR(S): 8055032: Improve numerical parsing in java.net and sun.net
Hi, the parseInt/parseLong API was changed[1] in accordance with the discussion in this thread. That big shuffling around of modules also happened, so I've updated this patch: http://cr.openjdk.java.net/~redestad/8055032/webrev.1/ /Claes [1] https://bugs.openjdk.java.net/browse/JDK-8055251 On 08/25/2014 10:25 AM, Alan Bateman wrote: On 18/08/2014 22:12, Mike Duigou wrote: On Aug 14 2014, at 06:39 , Alan Bateman alan.bate...@oracle.com wrote: On 14/08/2014 14:23, Claes Redestad wrote: How about methods only taking beginIndex? Integer.parseInt(x: 1000, 3, 10)? I guess these could to be dropped to avoid ambiguity and instead allow for variations where radix can be left out. I think there are two alternatives to the current implementation: - only keep parseInt(CharSequence s, int beginIndex, int endIndex, int radix) That's my preference Looking at the examples I agree that providing only this one method is probably the least error prone option. I think we mostly got agreement on net-dev to re-examine the newly introduced methods. I've created JDK-8055032 to track it and I'm sure Claes will pick it up once he gets back. -Alan.
Re: RFR(S): 8055032: Improve numerical parsing in java.net and sun.net
On 08/14/2014 09:37 AM, Alan Bateman wrote: On 13/08/2014 15:02, Claes Redestad wrote: Hi, can I have a review for this patch to take advantage of offset-based parseInt methods added in 8041972 for java.net/sun.net classes? bug: https://bugs.openjdk.java.net/browse/JDK-8055032 webrev: http://cr.openjdk.java.net/~redestad/8055032/webrev.0 This causes fewer temporary String objects to be allocated and shows a direct throughput improvement in micros (1.2x in java.net.URLDecoder#decode and sun.net.www.ParseUtil#decode, for example) These changes look okay to although the downside is that it's less readable in a few places. Thanks for looking at this, Alan! Any particular place where you think readability becomes a problem? I've grown fond of the parseInt(s, radix, offset) form myself, but I'm biased. ;-) Are there micro benchmarks being created as part of this work? If so, are they being pushed to a repository in OpenJDK for use by others? We currently don't have a suitable landing place for microbenchmarks in the OpenJDK. I hope to one day be able to push benchmarks together with changesets like these. I have a few internal benchmarks where I've extensively benchmarked the new parseInt methods and some sanity tests to ensure it gives reasonable benefits for public methods like java.net.URLDecoder::decode /Claes -Alan
Re: RFR(S): 8055032: Improve numerical parsing in java.net and sun.net
On 08/14/2014 01:30 PM, Alan Bateman wrote: On 14/08/2014 10:32, Claes Redestad wrote: : Any particular place where you think readability becomes a problem? I've grown fond of the parseInt(s, radix, offset) form myself, but I'm biased. ;-) It's somewhat subjective but when a method has a sequence of parameters that are the same type (int in this case) then reading the code requires a bit of extra effort to match up the parameters. Looking at this again then I just wonder if the radix should be last parameter rather than having it appear before the range. I think it would be clearer if the range were to follow the CharSequence rather than having the radix appear in the middle. I didn't have cycles to follow the recent discussion/review on core-libs-dev when these methods were added. Did the ordering come up? Noone brought it up, as far as I can recall. Since parseInt(String, int radix) already existed, I figured adding the range parameters to the end would be overall less awkward than to push the radix parameter right in the new methods. The chosen implementation maintains that the second parameter is always radix, which I think helps maintain consistency. /Claes -Alan.
Re: RFR(S): 8055032: Improve numerical parsing in java.net and sun.net
How about methods only taking beginIndex? Integer.parseInt(x: 1000, 3, 10)? I guess these could to be dropped to avoid ambiguity and instead allow for variations where radix can be left out. I think there are two alternatives to the current implementation: - only keep parseInt(CharSequence s, int beginIndex, int endIndex, int radix) - optional radix: parseInt(CharSequence s, int beginIndex, int endIndex), parseInt(CharSequence s, int beginIndex, int endIndex, int radix) (Should this discussion be moved to core-libs-dev?) /Claes On 08/14/2014 03:15 PM, roger riggs wrote: Hi, My preference would be to keep the offsets immediately following the CharSequence, that clearly identifies the substring being operated on. Roger On 8/14/2014 8:00 AM, Alan Bateman wrote: On 14/08/2014 12:42, Claes Redestad wrote: Noone brought it up, as far as I can recall. Since parseInt(String, int radix) already existed, I figured adding the range parameters to the end would be overall less awkward than to push the radix parameter right in the new methods. The chosen implementation maintains that the second parameter is always radix, which I think helps maintain consistency. I think consistency could be argued both ways as the radix is also the last parameter in the existing methods. When I look at this method: public static int parseInt(CharSequence s, int radix, int beginIndex, int endIndex) and then feels more error prone than if the beginIndex/endIndex were immediately after the CharSequence. I'm interested in other opinions on this. -Alan.
RFR(S): 8055032: Improve numerical parsing in java.net and sun.net
Hi, can I have a review for this patch to take advantage of offset-based parseInt methods added in 8041972 for java.net/sun.net classes? bug: https://bugs.openjdk.java.net/browse/JDK-8055032 webrev: http://cr.openjdk.java.net/~redestad/8055032/webrev.0 This causes fewer temporary String objects to be allocated and shows a direct throughput improvement in micros (1.2x in java.net.URLDecoder#decode and sun.net.www.ParseUtil#decode, for example) /Claes
