Re: RFR: 8272137: Make Iterable classes streamable
On Mon, 9 Aug 2021 12:28:23 GMT, CC007 wrote: > create Streamable and ParallelStreamable interface and use them in Collection > and Optional Aren't all iterable implementations effectively streamable if they properly implement `spliterator`? And the spliterator implementation can always be sequential or parallel, dependent on how you feed it into `StreamSupport`. - PR: https://git.openjdk.java.net/jdk/pull/5050
Re: RFR: 8272137: Make Iterable classes streamable
On Mon, 9 Aug 2021 12:28:23 GMT, CC007 wrote: > create Streamable and ParallelStreamable interface and use them in Collection > and Optional I couldn't think of any, but if there are more places where these interfaces could be used, please tell me, so I can add them. - PR: https://git.openjdk.java.net/jdk/pull/5050
Re: RFR: 8271302: Regex Test Refresh
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves wrote:
> 8271302: Regex Test Refresh
Changes requested by bchristi (Reviewer).
In the JBS issue, it looks like the Description was put in the Environment. :)
test/jdk/java/util/regex/RegExTest.java line 291:
> 289:
> 290: int resultStart1 = mr.start();
> 291: assertEquals(matcherStart1, resultStart1, "equal matchers have
> equal start indices");
Should the message be that they *don't* have equal start indices ?
test/jdk/java/util/regex/RegExTest.java line 2362:
> 2360:
> 2361: { "test\ud834\uddc0", "test\ud834\uddc0",
> "m", true },
> 2362: //{ "test\ud834\uddbc\ud834\udd6f", "test\ud834\uddc0",
> "m", true }, //problem
Should an issue be filed for these //problems ?
test/jdk/java/util/regex/RegExTest.java line 3952:
> 3950:
> 3951: m = Pattern.compile("\\H").matcher(matcherSubstring);
> 3952: assertTrue(m.find() || ng.equals(m.group()));
Should this be:
`assertTrue(m.find() && ng.equals(m.group()));`
-
PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8272137: Make Iterable classes streamable
On Mon, 9 Aug 2021 12:28:23 GMT, CC007 wrote: > create Streamable and ParallelStreamable interface and use them in Collection > and Optional Something seemed to have gone wrong with the jcheck - PR: https://git.openjdk.java.net/jdk/pull/5050
RFR: 8272137: Make Iterable classes streamable
create Streamable and ParallelStreamable interface and use them in Collection and Optional - Commit messages: - 8272137: no CR - 8272137: create Streamable and ParallelStreamable interface and use in Collection and Optional Changes: https://git.openjdk.java.net/jdk/pull/5050/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5050&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272137 Stats: 104 lines in 4 files changed: 101 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5050.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5050/head:pull/5050 PR: https://git.openjdk.java.net/jdk/pull/5050
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v4]
On Fri, 13 Aug 2021 19:20:48 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: Remove unneeded check of position and couunt src/java.base/share/classes/java/io/FileInputStream.java line 373: > 371: } > 372: return transferred + super.transferTo(out); > 373: } Good, I think this version is right. - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v4]
> 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: Remove unneeded check of position and couunt - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/796248e7..3e575ba2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5097&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5097&range=02-03 Stats: 8 lines in 1 file changed: 0 ins; 3 del; 5 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: 8272297: FileInputStream should override transferTo() for better performance [v3]
On Fri, 13 Aug 2021 14:53:45 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: Improve growing source and partial transfer cases
src/java.base/share/classes/java/io/FileInputStream.java line 367:
> 365: long count = fc.size() - pos;
> 366: if (pos >= 0 && count >= 0) {
> 367: transferred = fc.transferTo(pos, Long.MAX_VALUE,
> fos.getChannel());
This version looks better I think we can drop L365 and 366. The position cannot
be negative and checking that size >= pos isn't useful. In other words, you can
just call transferTo with the position without these checks.
-
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()
test/jdk/java/nio/file/spi/SetDefaultProvider.java line 26:
> 24: /**
> 25: * @test
> 26: * @bug 4313887 7006126 8142968 8178380 8183320 8210112 8266345 8263940
Thanks for correcting the @bug tag.
test/jdk/java/nio/file/spi/SetDefaultProvider.java line 89:
> 87: createFileSystemProviderJar(jar, Path.of(testClasses));
> 88: String classpath = jar + File.pathSeparator + testClasses
> 89: + File.separator + "modules" + File.separator + "m";
This ends up with two copies of TestFIleSystemProvider on the class path. I
think we should compile TestProvider to a different directory. That will
eliminate the need to filter the classes when creating the JAR file.
test/jdk/java/nio/file/spi/SetDefaultProvider.java line 99:
> 97: */
> 98: private void createFileSystemProviderJar(Path jar, Path dir) throws
> IOException {
> 99:
In this test, the supporting methods are at the end of the source file,
probably should keep it consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/5103
Integrated: 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 This pull request has now been integrated. Changeset: 717792c3 Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/717792c3b728584413572e7aede83290779be2a2 Stats: 55 lines in 2 files changed: 49 ins; 2 del; 4 mod 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path Reviewed-by: naoto, bpb, iris, joehw - PR: https://git.openjdk.java.net/jdk/pull/5103
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v3]
> 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: Improve growing source and partial transfer cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/5097/files - new: https://git.openjdk.java.net/jdk/pull/5097/files/a199fc67..796248e7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5097&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5097&range=01-02 Stats: 13 lines in 1 file changed: 1 ins; 3 del; 9 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: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Fri, 13 Aug 2021 14:32:41 GMT, Roman Kennke wrote: > Is this what you have been asking @mkarg in #4263 to do? Optimize > transferTo() only for FileInputStream? Would it interfere with #4263? #4263 is the input stream returned by Channels.newInputStream where the source may be a FileChannel and the output stream specified to InputStream::transferTo may be connected to a FileChannel. - PR: https://git.openjdk.java.net/jdk/pull/5097
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 There is no overlap with #4263. - PR: https://git.openjdk.java.net/jdk/pull/5097
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 Is this what you have been asking @mkarg in #4263 to do? Optimize transferTo() only for FileInputStream? Would it interfere with #4263? - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Fri, 13 Aug 2021 13:16:04 GMT, SkateScout
wrote:
> OK even if we keep out the edge case in the try block the
> "parseLong(nm.substring(index), radix)" could be replaced as already
> mentioned with parseLong(nm. index, nm.length(), radix)
I think it already was:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Long.java#L1287
> and in the catch block the idea to throw an "nice" error can be misleading.
> since -02147483648 for example would become -2147483648 because the radix is
> 8.
Yes, it's slightly misleading that the radix specifier gets cropped out, but
the error message does include the radix information so it's not a bug
technically:
jshell> Integer.decode("-01234567890123");
| Exception java.lang.NumberFormatException: For input string:
"-1234567890123" under radix 8
> Since per Radix only one String is possible to get through would if not be
> faster and more clear to check (compare) if it is the matching string and
> return the correct value else throw the error. This is also easy to read and
> even if is on the edge avoid substring , concationation and reparsing.
It might be a bit faster for that one non-exceptional accepted input, sure. It
could also incur a cost on the fast path due increasing the weight of the code.
You'd need to carefully measure that the added logic and checks doesn't cause
any trouble elsewhere.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 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. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable OK even if we keep out the edge case in the try block the "parseLong(nm.substring(index), radix)" could be replaced as already mentioned with parseLong(nm. index, nm.length(), radix) and in the catch block the idea to throw an "nice" error can be misleading. since -02147483648 for example would become -2147483648 because the radix is 8. Since per Radix only one String is possible to get through would if not be faster and more clear to check (compare) if it is the matching string and return the correct value else throw the error. This is also easy to read and even if is on the edge avoid substring , concationation and reparsing. - 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 > 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. I think the best course of action is to reduce the scope of this PR to the file channel cases. There is no reason why future PRs can't build on this and add implementations for other channel types. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Thu, 12 Aug 2021 21:26:08 GMT, SkateScout
wrote:
> Hi,
> i check Long.java line 1301...1311 and i am not sure if this code is really
> good.
>
> 1. If negative is false the whole part ends with two times the same
> substring and this implies the same error.
>
> 2. If negative is true and we get an error than we can throw the error
> without an second parse step or we can use the substring from the first round.
>
> 3. Also as mentioned above the parseLong(text,radix) should be changed to
> parseLong(seq, beginIndex, endIndex, radix)
>this would avoid at least in the positive case the substring at all.
>
> 4. The same points are with Integer as well.
>try {
>result = parseLong(nm.substring(index), radix);
>result = negative ? -result : result;
>} catch (NumberFormatException e) {
>// If number is Long.MIN_VALUE, we'll end up here. The next line
>// handles this case, and causes any genuine format error to be
>// rethrown.
>String constant = negative ? ("-" + nm.substring(index))
>: nm.substring(index);
>result = parseLong(constant, radix);
>}
The pre-existing logic here is iffy, but I think it is correct.
For Integer: If negative is true, then parsing "2147483648" (Integer.MAX_VALUE
+ 1) would throw, be reparsed as "-2147483648" and then be accepted as
Integer.MIN_VALUE. This is the only case that should be non-exceptional, but it
should also be exceedingly rare in practice. For negative values it improves
the error messages a bit to add the "-" and reparse.
Improving the catch clauses with `parseLong(CharSequence, int, int, int)` and
adding an `if (!negative) throw e` case to the catch could theoretically
improve performance of parsing the MIN_VALUE edge case and repeated decoding of
malformed positive numbers, but these are rare or exceptional cases where we
should favor simplicity over optimal performance
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 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.
>
> Сергей Цыпанов has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - 8267844: Add benchmarks for Integer/Long.decode()
> - 8267844: Rid useless substring when calling Integer/Long.parse*()
> - Merge branch 'master' into 8267844
> - Merge branch 'master' into 8267844
> - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where
> applicable
Hi,
i check Long.java line 1301...1311 and i am not sure if this code is really
good.
1) If negative is false the whole part ends with two times the same substring
and this implies the same error.
2) If negative is true and we get an error than we can throw the error without
an second parse step or we can use the substring from the first round.
3) Also as mentioned above the parseLong(text,radix) should be changed to
parseLong(seq, beginIndex, endIndex, radix)
this would avoid at least in the positive case the substring at all.
4) The same points are with Integer as well.
try {
result = parseLong(nm.substring(index), radix);
result = negative ? -result : result;
} catch (NumberFormatException e) {
// If number is Long.MIN_VALUE, we'll end up here. The next line
// handles this case, and causes any genuine format error to be
// rethrown.
String constant = negative ? ("-" + nm.substring(index))
: nm.substring(index);
result = parseLong(constant, radix);
}
-
PR: https://git.openjdk.java.net/jdk/pull/5068
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 src/java.base/share/classes/java/io/FileInputStream.java line 377: > 375: } > 376: } > 377: return super.transferTo(out); I think there is also another bug here for the case that transferTo does a partial transfer, it should be: return transferred + super.transferTo(out)); - PR: https://git.openjdk.java.net/jdk/pull/5097
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
src/java.base/share/classes/java/io/FileInputStream.java line 364:
> 362: FileChannel fci = getChannel();
> 363: long pos = fci.position();
> 364: long count = fci.size() - pos;
It might be better to just specify the count as Long.MAX_VALUE and test the
size after the transfer. This would be a bit more robust in the scenario that
the file grows and would avoid fallback when the file is truncated, e.g.
FileChannel fc = getChannel();
long pos = fc.position();
long transferred = fc.transferTo(pos, Long.MAX_VALUE, out.getChannel());
long newPos = pos + transferred;
fc.position(newPos);
if (newPos >= fc.size()) {
return transferred;
}
-
PR: https://git.openjdk.java.net/jdk/pull/5097
