Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test Without the source change the test fails, but passes with the change in place. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path [v2]
On Thu, 12 Aug 2021 19:27:42 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the fix for JDK-8263940 to address an issues when the default >> file system provider is packaged as JAR file on class path. >> >> The patch also addresses the `@bug` line for JDK-8271194 >> >> Mach5 Tier1 - Tier3 have run without issues >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Use toList() Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path [v2]
On Thu, 12 Aug 2021 19:27:42 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the fix for JDK-8263940 to address an issues when the default >> file system provider is packaged as JAR file on class path. >> >> The patch also addresses the `@bug` line for JDK-8271194 >> >> Mach5 Tier1 - Tier3 have run without issues >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Use toList() Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
> Please consider this request to add an override > `java.io.FileInputStream.transferTo(OutputStream)` with improved performance > if the parameter is a `FileOutputStream`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8272297: Set source position after FC.transferTo(); add test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/54f14550..a199fc67 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5097=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5097=00-01 Stats: 133 lines in 2 files changed: 132 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5097/head:pull/5097 PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8260265: UTF-8 by Default [v7]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Merge branch 'master' into JDK-8260265 - Refined `file.encoding` description - Merge branch 'master' into JDK-8260265 - Reflects comments - Removed leftover `console` references in `PrintStream` - Reflects comments - Reflects review comments - ... and 2 more: https://git.openjdk.java.net/jdk/compare/387b32b3...7d5137d3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/6f9e5eb4..7d5137d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=05-06 Stats: 53933 lines in 926 files changed: 45460 ins; 4145 del; 4328 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path [v2]
On Thu, 12 Aug 2021 18:55:08 GMT, Naoto Sato wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use toList()
>
> test/jdk/java/nio/file/spi/SetDefaultProvider.java line 107:
>
>> 105: .map(path -> path.getFileName().toString())
>> 106: .filter(f -> f.startsWith("TestProvider"))
>> 107: .collect(Collectors.toList());
>
> Nit: Could simply issue `.toList()` here.
Thank you Naoto, it is a bit cleaner so pushed the update.
-
PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path [v2]
On Thu, 12 Aug 2021 19:24:39 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the fix for JDK-8263940 to address an issues when the default >> file system provider is packaged as JAR file on class path. >> >> The patch also addresses the `@bug` line for JDK-8271194 >> >> Mach5 Tier1 - Tier3 have run without issues >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Use toList() Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path [v2]
> Hi all, > > Please review the fix for JDK-8263940 to address an issues when the default > file system provider is packaged as JAR file on class path. > > The patch also addresses the `@bug` line for JDK-8271194 > > Mach5 Tier1 - Tier3 have run without issues > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Use toList() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5103/files - new: https://git.openjdk.java.net/jdk/pull/5103/files/c09d7c32..6a65fb35 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5103=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5103=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5103.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5103/head:pull/5103 PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
On Thu, 12 Aug 2021 17:43:48 GMT, Lance Andersen wrote:
> Hi all,
>
> Please review the fix for JDK-8263940 to address an issues when the default
> file system provider is packaged as JAR file on class path.
>
> The patch also addresses the `@bug` line for JDK-8271194
>
> Mach5 Tier1 - Tier3 have run without issues
>
> Best,
> Lance
Looks good to me, Lance.
test/jdk/java/nio/file/spi/SetDefaultProvider.java line 107:
> 105: .map(path -> path.getFileName().toString())
> 106: .filter(f -> f.startsWith("TestProvider"))
> 107: .collect(Collectors.toList());
Nit: Could simply issue `.toList()` here.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Thu, 12 Aug 2021 08:40:34 GMT, Alan Bateman wrote: >> Markus KARG has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Draft: Eliminated duplicate code using lambda expressions >> - Draft: Use blocking mode also for target channel > >> I am a bit disappointed actually about that destructive answer at that late >> point in time, now that I worked for months on all the requested changes and >> tests. To prevent exactly this situation, I deliberately had the discussion >> started in JIRA only, and I deliberately had the original code being just a >> draft in the first place, and I deliberately did nearly _everything_ I was >> asked to (including even the most irrelevant minor code style issues). And >> you come up with the request to drop the code **now**? >> >> Certainly we could reduce the PR to just file channels, but in fact, now >> that I spent all the time in the non-file-channels, I wonder why I shall >> throw away all that work and go just with file channels actually? What is >> not covered that was originally covered, and what is that lots of issues you >> talk about? Actually I cannot see the actual problem unless you name it. > > There are 78 comments on this PR so far. We've tried to point out the bugs > and issues at each iteration. We asked for tests because the changes > introduce several code paths and implementations that would not be exercised > by existing tests. There are several scenarios still missing and the patch > doesn't yet have the microbenchmarks to demonstrate the improvements. > > I assume this is your first contribution so there will be learning curve and > maybe some frustration. I think you have a better chance of success if you > split this up and reduce the scope of this PR down to something manageable. > Keeping the selectable channels out of this PR and focusing on the case where > the input and output streams wrap file channels should make it simpler and > may lead to a better solution. Reducing the scope will also reduce the burden > on reviewers. > I do not know exactly what @AlanBateman had in mind, but I think there is > general concern about ensuring that all combinations of channel types and all > execution paths are exercised. @AlanBateman @bplb Does it make **any** real sense to answer your recent questions, provide the proofs, tests and benchmark results (I actually would love to *if* it makes sense) *or* will the outcome be that I *must* drop everything besides file channels *anyways* (In that case it is in vain)? As my time is just as precious as yours I really need to know that **before** I spend more weeks into code paths that you possibly decided to never accept ever. Don't get me wrong, if you see a chance to keep the code once I provided the answers I will do that, but if you do not see that chance, please frankly and unambiguously tell me **now**. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4263
RFR: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
Hi all, Please review the fix for JDK-8263940 to address an issues when the default file system provider is packaged as JAR file on class path. The patch also addresses the `@bug` line for JDK-8271194 Mach5 Tier1 - Tier3 have run without issues Best, Lance - Commit messages: - Fix for JDK-8263940 Changes: https://git.openjdk.java.net/jdk/pull/5103/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5103=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263940 Stats: 55 lines in 2 files changed: 49 ins; 2 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5103.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5103/head:pull/5103 PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 11:30:16 GMT, Alan Bateman wrote:
>> Please consider this request to add an override
>> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
>> if the parameter is a `FileOutputStream`.
>
> src/java.base/share/classes/java/io/FileInputStream.java line 371:
>
>> 369: // not, then use the superclass method to copy those
>> remaining
>> 370: if (fci.transferTo(pos, count, fco) == count) {
>> 371: return count;
>
> Are you sure this is right? FileChannel transferTo doesn't modify the source
> channel's position so I assume the method above will finish with the
> InputStream at its initial position rather than EOF. How are we on test
> coverage for this case?
You're right, that is stupid. I don't think it affected the benchmark, but
testing likely needs to be revisited.
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance
On Thu, 12 Aug 2021 01:16:04 GMT, Brian Burkhalter wrote:
> Please consider this request to add an override
> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance
> if the parameter is a `FileOutputStream`.
Changes requested by alanb (Reviewer).
src/java.base/share/classes/java/io/FileInputStream.java line 371:
> 369: // not, then use the superclass method to copy those
> remaining
> 370: if (fci.transferTo(pos, count, fco) == count) {
> 371: return count;
Are you sure this is right? FileChannel transferTo doesn't modify the source
channel's position so I assume the method above will finish with the
InputStream at its initial position rather than EOF. How are we on test
coverage for this case?
-
PR: https://git.openjdk.java.net/jdk/pull/5097
Integrated: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 13:16:42 GMT, Сергей Цыпанов wrote: > The code in `Integer.decode()` and `Long.decode()` might allocate two > instances of Integer/Long for the negative values less than -127: > > Integer result; > > result = Integer.valueOf(nm.substring(index), radix); > result = negative ? Integer.valueOf(-result.intValue()) : result; > > To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` > method. Same applicable for `Long` and some other classes. This pull request has now been integrated. Changeset: b29fbad9 Author:Sergey Tsypanov Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/b29fbad940808c59f30e60222a9ca7a23c8e54b9 Stats: 32 lines in 6 files changed: 19 ins; 1 del; 12 mod 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable Reviewed-by: redestad - PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sun, 1 Aug 2021 22:01:33 GMT, Markus KARG wrote: >> This PR-*draft* is **work in progress** and an invitation to discuss a >> possible solution for issue >> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not >> yet* intended for a final review. >> >> As proposed in JDK-8265891, this PR provides an implementation for >> `Channels.newInputStream().transferTo()` which provide superior performance >> compared to the current implementation. The changes are: >> * Prevents transfers through the JVM heap as much as possibly by offloading >> to deeper levels via NIO, hence allowing the operating system to optimize >> the transfer. >> * Using more JRE heap in the fallback case when no NIO is possible (still >> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern >> hardware / fast I/O devides. >> >> Using JMH I have benchmarked both, the original implementation and this >> implementation, and (depending on the used hardware and use case) >> performance change was approx. doubled performance. So this PoC proofs that >> it makes sense to finalize this work and turn it into an actual OpenJDK >> contribution. >> >> I encourage everybody to discuss this draft: >> * Are there valid arguments for *not* doing this change? >> * Is there a *better* way to improve performance of >> `Channels.newInputStream().transferTo()`? >> * How to go on from here: What is missing to get this ready for an actual >> review? > > Markus KARG has updated the pull request incrementally with two additional > commits since the last revision: > > - Draft: Eliminated duplicate code using lambda expressions > - Draft: Use blocking mode also for target channel > I am a bit disappointed actually about that destructive answer at that late > point in time, now that I worked for months on all the requested changes and > tests. To prevent exactly this situation, I deliberately had the discussion > started in JIRA only, and I deliberately had the original code being just a > draft in the first place, and I deliberately did nearly _everything_ I was > asked to (including even the most irrelevant minor code style issues). And > you come up with the request to drop the code **now**? > > Certainly we could reduce the PR to just file channels, but in fact, now that > I spent all the time in the non-file-channels, I wonder why I shall throw > away all that work and go just with file channels actually? What is not > covered that was originally covered, and what is that lots of issues you talk > about? Actually I cannot see the actual problem unless you name it. There are 78 comments on this PR so far. We've tried to point out the bugs and issues at each iteration. We asked for tests because the changes introduce several code paths and implementations that would not be exercised by existing tests. There are several scenarios still missing and the patch doesn't yet have the microbenchmarks to demonstrate the improvements. I assume this is your first contribution so there will be learning curve and maybe some frustration. I think you have a better chance of success if you split this up and reduce the scope of this PR down to something manageable. Keeping the selectable channels out of this PR and focusing on the case where the input and output streams wrap file channels should make it simpler and may lead to a better solution. Reducing the scope will also reduce the burden on reviewers. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Wed, 11 Aug 2021 11:27:53 GMT, Alan Bateman wrote: >>> I think I fixed all requested changes. Anymore comments on this PR? >> >> I hope to get to this soon. > >> I think I fixed all requested changes. Anymore comments on this PR? > > I've looked through the latest revision. Is there any way that we could drop > most of the changes to ChannelInputStream and focus on one or two specific > cases? I'm asking because there are several issues, inconsistencies, and it > is trying to cover many scenarios that aren't covered by the test. > > If the original motivation was file -> file then it could be simplified down > to a FileChannel -> FileChannel transfer as the default provider uses file > channels. We could even push some support into FileChannelImpl so that it is > done while holding the position lock. I do not know exactly what @AlanBateman had in mind, but I think there is general concern about ensuring that all combinations of channel types and all execution paths are exercised. - PR: https://git.openjdk.java.net/jdk/pull/4263
Integrated: 8271732: Regression in StringBuilder.charAt bounds checking
On Wed, 11 Aug 2021 14:26:32 GMT, Claes Redestad wrote: > In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to > avoid potentially getting two bounds checks in the UTF-16 case. Problem is > this optimization cause a regression since `StringUTF16.charAt(..)` checks > `index` against the length of the `value` array and not `count`. > > A correct fix that still maintain the possible performance advantage reported > by #4738 is to reinstate the `checkIndex(value, count)` and call > `StringUTF16.getChar` instead of `charAt`. This pull request has now been integrated. Changeset: a15b6592 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/a15b659278741337aefc15ce8002df66ce6323c0 Stats: 108 lines in 3 files changed: 106 ins; 0 del; 2 mod 8271732: Regression in StringBuilder.charAt bounds checking Reviewed-by: alanb, naoto - PR: https://git.openjdk.java.net/jdk/pull/5086
Re: RFR: 8271732: Regression in StringBuilder.charAt bounds checking [v4]
On Wed, 11 Aug 2021 21:40:52 GMT, Claes Redestad wrote: >> In #4738 we removed the `checkIndex(value, count)` call in `ASB.charAt` to >> avoid potentially getting two bounds checks in the UTF-16 case. Problem is >> this optimization cause a regression since `StringUTF16.charAt(..)` checks >> `index` against the length of the `value` array and not `count`. >> >> A correct fix that still maintain the possible performance advantage >> reported by #4738 is to reinstate the `checkIndex(value, count)` and call >> `StringUTF16.getChar` instead of `charAt`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copy-paste error Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5086
