Re: RFR: 8266054: VectorAPI rotate operation optimization [v13]
On Tue, 20 Jul 2021 09:57:07 GMT, Jatin Bhateja wrote: >> Current VectorAPI Java side implementation expresses rotateLeft and >> rotateRight operation using following operations:- >> >> vec1 = lanewise(VectorOperators.LSHL, n) >> vec2 = lanewise(VectorOperators.LSHR, n) >> res = lanewise(VectorOperations.OR, vec1 , vec2) >> >> This patch moves above handling from Java side to C2 compiler which >> facilitates dismantling the rotate operation if target ISA does not support >> a direct rotate instruction. >> >> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over >> long and integer type vectors. For other cases (i.e. sub-word type vectors >> or for targets which do not support direct rotate operations ) instruction >> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted. >> >> Please find below the performance data for included JMH benchmark. >> Machine: Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz) >> >> >> > xmlns:o="urn:schemas-microsoft-com:office:office" >> xmlns:x="urn:schemas-microsoft-com:office:excel" >> xmlns="http://www.w3.org/TR/REC-html40;> >> >> >> >> >> >> > href="file:///C:/Users/jatinbha/AppData/Local/Temp/msohtmlclip1/01/clip.htm"> >> > href="file:///C:/Users/jatinbha/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml"> >> >> >> >> >> >> >> >> >> >> Benchmark | (bits) | (shift) | (size) | Baseline Score (ops/ms) | With Opts >> (ops/ms) | Gain >> -- | -- | -- | -- | -- | -- | -- >> RotateBenchmark.testRotateLeftB | 128 | 7 | 256 | 3939.136 | 3836.133 | >> 0.973851372 >> RotateBenchmark.testRotateLeftB | 128 | 7 | 512 | 1984.231 | 1918.27 | >> 0.966757399 >> RotateBenchmark.testRotateLeftB | 128 | 15 | 256 | 3925.165 | 4043.842 | >> 1.030234907 >> RotateBenchmark.testRotateLeftB | 128 | 15 | 512 | 1962.723 | 1936.551 | >> 0.986665464 >> RotateBenchmark.testRotateLeftB | 128 | 31 | 256 | 3945.6 | 3817.883 | >> 0.967630525 >> RotateBenchmark.testRotateLeftB | 128 | 31 | 512 | 1944.458 | 1914.229 | >> 0.984453766 >> RotateBenchmark.testRotateLeftB | 256 | 7 | 256 | 4612.149 | 4514.874 | >> 0.978908964 >> RotateBenchmark.testRotateLeftB | 256 | 7 | 512 | 2296.252 | 2270.237 | >> 0.988670669 >> RotateBenchmark.testRotateLeftB | 256 | 15 | 256 | 4576.628 | 4515.53 | >> 0.986649996 >> RotateBenchmark.testRotateLeftB | 256 | 15 | 512 | 2288.278 | 2270.923 | >> 0.992415694 >> RotateBenchmark.testRotateLeftB | 256 | 31 | 256 | 4624.243 | 4511.46 | >> 0.975610495 >> RotateBenchmark.testRotateLeftB | 256 | 31 | 512 | 2305.459 | 2273.788 | >> 0.986262605 >> RotateBenchmark.testRotateLeftB | 512 | 7 | 256 | 7748.283 | .105 | >> 1.003719792 >> RotateBenchmark.testRotateLeftB | 512 | 7 | 512 | 3906.214 | 3912.647 | >> 1.001646863 >> RotateBenchmark.testRotateLeftB | 512 | 15 | 256 | 7764.653 | 7763.482 | >> 0.999849188 >> RotateBenchmark.testRotateLeftB | 512 | 15 | 512 | 3916.061 | 3919.363 | >> 1.000843194 >> RotateBenchmark.testRotateLeftB | 512 | 31 | 256 | 7779.754 | 7770.239 | >> 0.998776954 >> RotateBenchmark.testRotateLeftB | 512 | 31 | 512 | 3916.471 | 3912.718 | >> 0.999041739 >> RotateBenchmark.testRotateLeftI | 128 | 7 | 256 | 4043.39 | 13461.814 | >> 3.329338501 >> RotateBenchmark.testRotateLeftI | 128 | 7 | 512 | 1996.217 | 6455.425 | >> 3.233829288 >> RotateBenchmark.testRotateLeftI | 128 | 15 | 256 | 4028.614 | 13077.277 | >> 3.246098286 >> RotateBenchmark.testRotateLeftI | 128 | 15 | 512 | 1997.612 | 6452.918 | >> 3.230315997 >> RotateBenchmark.testRotateLeftI | 128 | 31 | 256 | 4123.357 | 13079.045 | >> 3.171940969 >> RotateBenchmark.testRotateLeftI | 128 | 31 | 512 | 2003.356 | 6452.716 | >> 3.22095324 >> RotateBenchmark.testRotateLeftI | 256 | 7 | 256 | 7666.949 | 25658.625 | >> 3.34665393 >> RotateBenchmark.testRotateLeftI | 256 | 7 | 512 | 3855.826 | 12278.106 | >> 3.18429981 >> RotateBenchmark.testRotateLeftI | 256 | 15 | 256 | 7670.901 | 24625.466 | >> 3.210244272 >> RotateBenchmark.testRotateLeftI | 256 | 15 | 512 | 3765.786 | 12272.771 | >> 3.259019764 >> RotateBenchmark.testRotateLeftI | 256 | 31 | 256 | 7660.599 | 25678.864 | >> 3.352069988 >> RotateBenchmark.testRotateLeftI | 256 | 31 | 512 | 3773.401 | 12006.469 | >> 3.181869353 >> RotateBenchmark.testRotateLeftI | 512 | 7 | 256 | 11900.948 | 31242.989 | >> 2.625252123 >> RotateBenchmark.testRotateLeftI | 512 | 7 | 512 | 5830.878 | 15727.149 | >> 2.697217983 >> RotateBenchmark.testRotateLeftI | 512 | 15 | 256 | 12171.847 | 33180.067 | >> 2.72596813 >> RotateBenchmark.testRotateLeftI | 512 | 15 | 512 | 5830.544 | 16740.182 | >> 2.871118372 >> RotateBenchmark.testRotateLeftI | 512 | 31 | 256 | 11909.553 | 31250.882 | >> 2.624018047 >> RotateBenchmark.testRotateLeftI | 512 | 31 | 512 | 5846.747 | 15738.831 | >> 2.691895339 >> RotateBenchmark.testRotateLeftL | 128 | 7 | 256 | 2047.243 | 6888.484 | >> 3.364761291 >> RotateBenchmark.testRotateLeftL | 128 | 7 | 512 | 1005.029 | 3245.931
Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]
On Mon, 26 Jul 2021 18:56:01 GMT, Jatin Bhateja wrote: >> @jatin-bhateja This question is still pending. > > @sviswa7, SLP flow will either have a constant 8bit shift value or a variable > shift present in vector. So non constant scalar case will not be hit through > this route. It would be better comment here, since the correctness relay on some others. - PR: https://git.openjdk.java.net/jdk/pull/3720
Integrated: 8269753: Misplaced caret in PatternSyntaxException's detail message
On Mon, 26 Jul 2021 18:10:03 GMT, Ian Graves wrote: > Fixes a bug where carets aren't indented correctly in PatternSyntaxException > messages because tab characters are converted to spaces in their indentation. This pull request has now been integrated. Changeset: bb508e13 Author:Ian Graves URL: https://git.openjdk.java.net/jdk/commit/bb508e13032c3571c48275391dfeb04c03bbf3a3 Stats: 24 lines in 2 files changed: 21 ins; 0 del; 3 mod 8269753: Misplaced caret in PatternSyntaxException's detail message Reviewed-by: prappo - PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Mon, 26 Jul 2021 20:45:14 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 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. test/jdk/sun/nio/ch/ChannelInputStream/TransferTo.java line 67: > 65: test(readableByteChannelInput(), defaultOutput()); > 66: } > 67: This test looks like it's doing what it's supposed to do. I'm not asking to change it, but I think using TestNG might have given a simpler result with less work. For example, there could be one `DataProvider` supplying inputs which feed a `@Test` which expects an NPE, and another `DataProvider` supplying input-output pairs which feeds a `@Test` which validates the functionality. - PR: https://git.openjdk.java.net/jdk/pull/4263
Integrated: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
On Mon, 28 Jun 2021 03:41:20 GMT, Jaikiran Pai wrote: > Can I please get a review for this proposed fix for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8190753? > > The commit here checks for the size of the zip entry before trying to create > a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been > included in this commit to reproduce the issue and verify the fix. > > P.S: It's still a bit arguable whether it's a good idea to create the > `ByteArrayOutputStream` with an initial size potentially as large as the > `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default > value. However, I think that can be addressed separately while dealing with > https://bugs.openjdk.java.net/browse/JDK-8011146 This pull request has now been integrated. Changeset: c3d8e922 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/c3d8e9228d0558a2ce3e093c105c61ea7af2e1d1 Stats: 356 lines in 3 files changed: 350 ins; 0 del; 6 mod 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream Reviewed-by: lancea - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
On Fri, 23 Jul 2021 14:58:01 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8190753? >> >> The commit here checks for the size of the zip entry before trying to create >> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has >> been included in this commit to reproduce the issue and verify the fix. >> >> P.S: It's still a bit arguable whether it's a good idea to create the >> `ByteArrayOutputStream` with an initial size potentially as large as the >> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default >> value. However, I think that can be addressed separately while dealing with >> https://bugs.openjdk.java.net/browse/JDK-8011146 > > Jaikiran Pai 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 15 additional > commits since the last revision: > > - Merge latest from master branch > - Implement review suggestions: > - remove unnecessary "synchronized" > - no need to open the temp file twice > - remove no longer necessary javadoc comment on test > - review suggestion - wrap ByteArrayOutputStream instead of extending it > - reorganize the tests now that the temp file creation threshold isn't > exposed as a user configurable value > - minor update to comment on FileRolloverOutputStream class > - remove no longer used constant > - use jtreg's construct of manual test > - Implement Alan's review suggestion - don't expose the threshold as a > configuration property, instead set it internally to a specific value > - propagate back the original checked IOException to the callers > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/f875aac8...991de6b9 Thank you for running the tests, Lance. - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Mon, 26 Jul 2021 20:45:14 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 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. src/java.base/share/classes/sun/nio/ch/ChannelOutputStream.java line 85: > 83: private byte[] bs; // Invoker's previous array > 84: private byte[] b1; > 85: It might be better to put the field declarations at the beginning of the class before the static methods. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
On Mon, 26 Jul 2021 20:45:14 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 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. src/java.base/share/classes/sun/nio/ch/ChannelInputStream.java line 179: > 177: for (long n = srcSize - srcPos; bytesWritten < n;) > 178: bytesWritten += src.transferTo(srcPos + bytesWritten, > Long.MAX_VALUE, dest); > 179: return bytesWritten; If `src` is extended at an inconvenient point in time, for example between the return from a call to `src.transferTo()` that makes `bytesWritten < n` false and the evaluation of that terminating condition, then it appears that not all the content of `src` will be transferred to `dest`. I am not however sure that this violates any contract but it could be a behavioral change from the existing implementation. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message [v3]
On Mon, 26 Jul 2021 20:54:53 GMT, Ian Graves wrote: >> Fixes a bug where carets aren't indented correctly in PatternSyntaxException >> messages because tab characters are converted to spaces in their indentation. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Tweaking some spacing. Fixing some report placement. Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
On Fri, 23 Jul 2021 16:22:01 GMT, Lance Andersen wrote: > > Thank you for the review Alan. > > @LanceAndersen, I've run the tier1 tests locally with the latest PR and > > they have passed without any regressions. Given that we changed the > > implementation to wrap ByteArrayOutputStream instead of extending it, would > > you want to rerun some of your other tests that you had previously run, > > before I integrate this? > > Yes, I will run additional tests and report back after they complete I did not notice any new issues after running tier1 - tier3 - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v9]
On Mon, 26 Jul 2021 03:28:44 GMT, Lin Zang wrote:
>> 4890732: GZIPOutputStream doesn't support optional GZIP fields
>
> Lin Zang has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 13 commits:
>
> - change since version to 18
> - Merge branch 'master' into gzip-field
> - Merge branch 'master' into gzip-field
> - Add api in GZIPInputStream to get header data
> - Merge remote-tracking branch 'upstream/master' into gzip-field
> - remove trailing spaces
> - Use record and Builder pattern
> - add class GZIPHeaderData, refine testcases
> - update copyright
> - reuse arguments constructor for non-argument one.
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/e627caec...b1868e8f
Thank you for reviving the discussion.
I have not gone through the latest update in detail but there are some changes
that are needed. Before moving forward with the CSR, I would like to give time
for additional feedback on naming and design.
I am not sure the builder names withXXX are the preferred naming pattern.
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 33:
> 31: * @author Lin Zang
> 32: * @since 18
> 33: *
The overview section needs more detail and examples of the usage
Please remove the author tag as we have moved away from this tag (you can see
who was involved via the file history
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 114:
> 112: */
> 113: public GZIPHeaderBuilder withFileComment(String fileComment) {
> 114: if (fileComment == null || fileComment.length() == 0) {
What happens if the String contains characters outside of ISO_8859_1?
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 136:
> 134: return this;
> 135: }
> 136:
Is this really needed as a public method ?
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 74:
> 72: int size,
> 73: boolean syncFlush,
> 74: GZIPHeaderBuilder.GZIPHeaderData
> gzipHeaderData)
Given the Gzip header data is so infrequently modified, one additional
constructor is all that really need.
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 91:
> 89: * Creates a new output stream with the specified buffer size and
> 90: * flush mode. And leave all other header fields set to default value.
> 91: *
This needs rewording as we do not typically start a sentence with "And..."
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 113:
> 111: /**
> 112: * Creates a new output stream with the specified buffer size.
> 113: * And leave all other header fields set to default value.
Same comment regarding "And..."
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 144:
> 142: /**
> 143: * Creates a new output stream with a default buffer size and
> 144: * the specified flush mode. And leave all other header fields
Same comment regarding "And..."
-
Changes requested by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: JDK-8256844: Make NMT late-initializable
Hi Thomas, On 26/07/2021 10:15 pm, Thomas Stuefe wrote: Short: this patch makes NMT available in custom-launcher scenarios and during gtests. It simplifies NMT initialization. It adds a lot of NMT-specific testing. Before looking at this, have you checked the startup performance impact? Thanks, David - - NMT continues to be an extremely useful tool for SAP to tackle memory problems in the JVM. However, NMT is of limited use due to the following restrictions: - NMT cannot be used if the hotspot is embedded into a custom launcher unless the launcher actively cooperates. Just creating and invoking the JVM is not enough, it needs to do some steps prior to loading the hotspot. This limitation is not well known (nor, do I believe, documented). Many products don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this problem limits NMT usefulness greatly since our VMs are often embedded into custom launchers and modifying every launcher is impossible. - Worse, if that custom launcher links the libjvm *statically* there is just no way to activate NMT at all. This is the reason NMT cannot be used in the `gtestlauncher`. - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` and `-XX:Flags=`. - The fact that NMT cannot be used in gtests is really a pity since it would allow us to both test NMT itself more rigorously and check for memory leaks while testing other stuff. The reason for all this is that NMT initialization happens very early, on the first call to `os::malloc()`. And those calls happen already during dynamic C++ initialization - a long time before the VM gets around parsing arguments. So, regular VM argument parsing is too late to parse NMT arguments. The current solution is to pass NMT arguments via a specially prepared environment variable: `NMT_LEVEL_=`. That environment variable has to be set by the embedding launcher, before it loads the libjvm. Since its name contains the PID, we cannot even set that variable in the shell before starting the launcher. All that means that every launcher needs to especially parse and process the NMT arguments given at the command line (or via whatever method) and prepare the environment variable. `java` itself does this. This only works before the libjvm.so is loaded, before its dynamic C++ initialization. For that reason, it does not work if the launcher links statically against the hotspot, since in that case C++ initialization of the launcher and hotspot are folded into one phase with no possibility of executing code beforehand. And since it bypasses argument handling in the VM, it bypasses a number of argument processing ways, e.g., `JAVA_TOOL_OPTIONS`. -- This patch fixes these shortcomings by making NMT late-initializable: it can now be initialized after normal VM argument parsing, like all other parts of the VM. This greatly simplifies NMT initialization and makes it work automagically for every third party launcher, as well as within our gtests. The glaring problem with late-initializing NMT is the NMT malloc headers. If we rule out just always having them (unacceptable in terms of memory overhead), there is no safe way to determine, in os::free(), if an allocation came from before or after NMT initialization ran, and therefore what to do with its malloc headers. For a more extensive explanation, please see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and @zhengyu123 in the JBS comment section. The heart of this patch is a new way to track early, pre-NMT-init allocations. These are tracked via a lookup table. This was a suggestion by Kim and it worked out well. Changes in detail: - pre-NMT-init handling: - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init handling. They contain a small global lookup table managing C-heap blocks allocated in the pre-NMT-init phase. - `os::malloc()/os::realloc()/os::free()` defer to this code before doing anything else. - Please see the extensive comment block at the start of `nmtPreinit.hpp` explaining the details. - Changes to NMT: - Before, NMT initialization was spread over two phases, `initialize()` and `late_initialize()`. Those were merged into one and simplified - there is only one initialization now which happens after argument parsing. - Minor changes were needed for the `NMT_TrackingLevel` enum - to simplify code, I changed NMT_unknown to be numerically 0. A new comment block in `nmtCommon.hpp` now clearly specifies what's what, including allowed level state transitions. - New utility functions to translate tracking level from/to strings added to `NMTUtil` - NMT has never been able to handle virtual memory allocations before initialization, which is fine since os::reserve_memory() is not called before VM parses arguments. We now assert that. - All code outside the VM handling NMT initialization
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message [v3]
> Fixes a bug where carets aren't indented correctly in PatternSyntaxException > messages because tab characters are converted to spaces in their indentation. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Tweaking some spacing. Fixing some report placement. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4906/files - new: https://git.openjdk.java.net/jdk/pull/4906/files/339b438a..c47b0651 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4906=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4906=01-02 Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4906.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4906/head:pull/4906 PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]
> 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 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. The pull request contains one new commit since the last revision: Draft: Test for ChannelInputStream::transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/bfc699a0..1f9dba3d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4263=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4263=04-05 Stats: 173 lines in 1 file changed: 6 ins; 0 del; 167 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v5]
> 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 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. The pull request contains one new commit since the last revision: Draft: Test for ChannelInputStream::transferTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/b431fcd6..bfc699a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4263=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4263=03-04 Stats: 227 lines in 1 file changed: 0 ins; 0 del; 227 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310) and generally fixes the footprint regression, it may > increase MaxRSS slightly compared to the version before JDK-8266310, which is > shown in the below graphs. (updated) > >  > >  > > (update: added ZGC graphs) > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4893
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message [v2]
> Fixes a bug where carets aren't indented correctly in PatternSyntaxException > messages because tab characters are converted to spaces in their indentation. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/4906/files - new: https://git.openjdk.java.net/jdk/pull/4906/files/2609dd96..339b438a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4906=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4906=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4906.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4906/head:pull/4906 PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/SystemDictionaryHelper.java line 92: > 90: } > 91: > 92: InstanceKlass[] searchResult = tmp.toArray(new InstanceKlass[0]); Is it too far outside the scope of these changes to make `tmp` an `ArrayList` rather than a `Vector`? - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message
On Mon, 26 Jul 2021 18:10:03 GMT, Ian Graves wrote:
> Fixes a bug where carets aren't indented correctly in PatternSyntaxException
> messages because tab characters are converted to spaces in their indentation.
Changes requested by prappo (Reviewer).
src/java.base/share/classes/java/util/regex/PatternSyntaxException.java line
111:
> 109: sb.append(System.lineSeparator());
> 110: for (int i = 0; i < index; i++) {
> 111: sb.append((pattern.charAt(i) == '\t') ? '\t' : ' ');
In contrast with
https://github.com/igraves/jdk/blob/2609dd9618dd43ea0de9abe3e3100262d09c079c/src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractDiagnosticFormatter.java#L324,
this code uses `StringBuilder.append(char)`, which might be even cleaner; good.
test/jdk/java/util/regex/RegExTest.java line 5304:
> 5302: var message = e.getMessage();
> 5303: var sep = System.lineSeparator();
> 5304: if(message.contains(sep + "\t ^")){
Suggestion:
if (message.contains(sep + "\t ^")) {
test/jdk/java/util/regex/RegExTest.java line 5310:
> 5308: failCount++;
> 5309:
> 5310: report("Correct caret indentation for patterns with tabs");
`report` will not be reached on success; this is different from how `report` is
used in the rest of the file. Also: `report` seems to be stateful in that it
resets the `failCount`.
-
PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. src/java.base/share/classes/java/security/Security.java line 656: > 654: return null; > 655: > 656: return candidates.toArray(new Provider[0]); Is this called often enough to warrant creating a constant of `new Provider[0]` (benefits of this covered in the _Wisdom of the Ancients_ blog linked)? - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. The changes to the security classes look good. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]
On Mon, 26 Jul 2021 17:19:07 GMT, Sandhya Viswanathan wrote: >> And'ing with shift_mask is already done on Java API side implementation >> before making a call to intrinsic rountine. > > @jatin-bhateja This question is still pending. @sviswa7, SLP flow will either have a constant 8bit shift value or a variable shift present in vector. So non constant scalar case will not be hit through this route. - PR: https://git.openjdk.java.net/jdk/pull/3720
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310) and generally fixes the footprint regression, it may > increase MaxRSS slightly compared to the version before JDK-8266310, which is > shown in the below graphs. (updated) > >  > >  > > (update: added ZGC graphs) > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. Hi Sergey, thanks for the explanation, i don't think there is a need for more data with -Xint. Using anonymous classes instead of lambdas is good for me. - PR: https://git.openjdk.java.net/jdk/pull/4893
Integrated: 8268873: Unnecessary Vector usage in java.base
On Mon, 14 Jun 2021 11:34:50 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where `Vector` was used as local variable. This pull request has now been integrated. Changeset: b8f79a7f Author:Andrey Turbanov Committer: Sean Mullan URL: https://git.openjdk.java.net/jdk/commit/b8f79a7ff798d3a0eee03a8153be942401781bbc Stats: 18 lines in 3 files changed: 1 ins; 4 del; 13 mod 8268873: Unnecessary Vector usage in java.base Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message
On Mon, 26 Jul 2021 18:10:03 GMT, Ian Graves wrote: > Fixes a bug where carets aren't indented correctly in PatternSyntaxException > messages because tab characters are converted to spaces in their indentation. @pavelrappo - PR: https://git.openjdk.java.net/jdk/pull/4906
RFR: 8269753: Misplaced caret in PatternSyntaxException's detail message
Fixes a bug where carets aren't indented correctly in PatternSyntaxException messages because tab characters are converted to spaces in their indentation. - Commit messages: - 8269753: Misplaced caret in PatternSyntaxException's detail message Changes: https://git.openjdk.java.net/jdk/pull/4906/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4906=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269753 Stats: 23 lines in 2 files changed: 21 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4906.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4906/head:pull/4906 PR: https://git.openjdk.java.net/jdk/pull/4906
Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]
On Mon, 26 Jul 2021 17:19:07 GMT, Sandhya Viswanathan wrote: >> And'ing with shift_mask is already done on Java API side implementation >> before making a call to intrinsic rountine. > > @jatin-bhateja This question is still pending. Other than VectorAPI , SLP also infers vector rotates where shift is either a 8bit constant or variable shift present in vector. So this case of scalar non-constant shift will not be hit for non-vectorAPI case. Also it will be illegal to perform any wrap around for shifts. - PR: https://git.openjdk.java.net/jdk/pull/3720
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 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 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. Sorry for the delay. I now have pushed a first draft of the test. IIUC then five implementation have to be tested just for the actualy API, i. e. whether they throws NPE when `out` is `null`, and whether they transfer small, single turn, and multiple turn loads correctly. The tests are taken from `InputStream/TransferTo.java` as you proposed, and I am not using mocking but instead use `Channels.new*` as the existing tests you proposed as a blueprint. I hope I understood your requests and proposals correctly, if not please tell me. The tests pass well and proof that the new implementations work as expected and API-compliant. - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v4]
> 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 four additional commits since the last revision: - Draft: Test for ChannelInputStream::transferTo - Draft: Using Channels API to invoke sun.nio.ch classes - Draft: MUST check params before ANY other operation - Draft: ChannelInputStream::transferTo Test - Changes: - all: https://git.openjdk.java.net/jdk/pull/4263/files - new: https://git.openjdk.java.net/jdk/pull/4263/files/47ee00a2..b431fcd6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4263=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4263=02-03 Stats: 229 lines in 2 files changed: 229 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4263.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263 PR: https://git.openjdk.java.net/jdk/pull/4263
Integrated: 8075806: divideExact is missing in java.lang.Math
On Tue, 13 Jul 2021 17:21:52 GMT, Brian Burkhalter wrote: > Please consider this proposal to add `divideExact()` methods for integral > data types to `java.lang.Math` thereby rounding out "exact" support to all > four basic arithmetic operations. This pull request has now been integrated. Changeset: 0b12e7c8 Author:Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/0b12e7c82c559f64c8c202bf59ee71f9cbd5a5fa Stats: 177 lines in 3 files changed: 157 ins; 10 del; 10 mod 8075806: divideExact is missing in java.lang.Math Reviewed-by: darcy - PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]
On Sun, 18 Jul 2021 20:22:18 GMT, Jatin Bhateja wrote: >> src/hotspot/share/opto/vectornode.cpp line 1180: >> >>> 1178: cnt = cnt->in(1); >>> 1179: } >>> 1180: shiftRCnt = cnt; >> >> Why do we remove the And with mask here? > > And'ing with shift_mask is already done on Java API side implementation > before making a call to intrinsic rountine. @jatin-bhateja This question is still pending. - PR: https://git.openjdk.java.net/jdk/pull/3720
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310) and generally fixes the footprint regression, it may > increase MaxRSS slightly compared to the version before JDK-8266310, which is > shown in the below graphs. (updated) > >  > >  > > (update: added ZGC graphs) > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. NativeLibraries was called early during VM startup and the startup benchmark is just a Noop. So the lambda creation by NativeLibraries is likely still running in interpreted mode. Looks like replacing it with an anonymous class may be an alternative. I ran a few startup benchmarks on linux x64 with this patch. It does show that the startup regression is fixed and also the footprint benchmark I included does not show any regression. - PR: https://git.openjdk.java.net/jdk/pull/4893
Integrated: 8171382: java.time.Duration missing isPositive method
On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato wrote: > Please review this PR to introduce `java.time.Duration.isPositive()` method. > A CSR is also drafted. This pull request has now been integrated. Changeset: efa63dc1 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/efa63dc1c64db357eeb497d2e1fefd170ca22d98 Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod 8171382: java.time.Duration missing isPositive method Reviewed-by: rriggs, joehw, iris, bpb, scolebourne - PR: https://git.openjdk.java.net/jdk/pull/4892
Integrated: 8265474: Dubious 'null' assignment in CompactByteArray.expand
On Wed, 23 Dec 2020 16:06:30 GMT, Andrey Turbanov wrote: > I propose to remove 'null' assignment of field CompactByteArray.values in > `expand` method. In the next statement this field is overridden with another > value - `tempArray`. > This code was there from initial load of OpenJDK sources. I believe it was > just leftovers from development phase of this class. There is no practical > reason to assign `null` to non-volatile field and then overwrite it with > another value. > Also I've removed unused method `getArray`. I hope it's ok to cleanup such > unused stuff in the same PR. This pull request has now been integrated. Changeset: ee553618 Author:Andrey Turbanov Committer: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/ee5536183a9df90d1209d9effe5d2aa61d86abd3 Stats: 9 lines in 1 file changed: 0 ins; 6 del; 3 mod 8265474: Dubious 'null' assignment in CompactByteArray.expand Reviewed-by: alanb, naoto - PR: https://git.openjdk.java.net/jdk/pull/1880
RFR: JDK-8256844: Make NMT late-initializable
Short: this patch makes NMT available in custom-launcher scenarios and during gtests. It simplifies NMT initialization. It adds a lot of NMT-specific testing. - NMT continues to be an extremely useful tool for SAP to tackle memory problems in the JVM. However, NMT is of limited use due to the following restrictions: - NMT cannot be used if the hotspot is embedded into a custom launcher unless the launcher actively cooperates. Just creating and invoking the JVM is not enough, it needs to do some steps prior to loading the hotspot. This limitation is not well known (nor, do I believe, documented). Many products don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this problem limits NMT usefulness greatly since our VMs are often embedded into custom launchers and modifying every launcher is impossible. - Worse, if that custom launcher links the libjvm *statically* there is just no way to activate NMT at all. This is the reason NMT cannot be used in the `gtestlauncher`. - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` and `-XX:Flags=`. - The fact that NMT cannot be used in gtests is really a pity since it would allow us to both test NMT itself more rigorously and check for memory leaks while testing other stuff. The reason for all this is that NMT initialization happens very early, on the first call to `os::malloc()`. And those calls happen already during dynamic C++ initialization - a long time before the VM gets around parsing arguments. So, regular VM argument parsing is too late to parse NMT arguments. The current solution is to pass NMT arguments via a specially prepared environment variable: `NMT_LEVEL_=`. That environment variable has to be set by the embedding launcher, before it loads the libjvm. Since its name contains the PID, we cannot even set that variable in the shell before starting the launcher. All that means that every launcher needs to especially parse and process the NMT arguments given at the command line (or via whatever method) and prepare the environment variable. `java` itself does this. This only works before the libjvm.so is loaded, before its dynamic C++ initialization. For that reason, it does not work if the launcher links statically against the hotspot, since in that case C++ initialization of the launcher and hotspot are folded into one phase with no possibility of executing code beforehand. And since it bypasses argument handling in the VM, it bypasses a number of argument processing ways, e.g., `JAVA_TOOL_OPTIONS`. -- This patch fixes these shortcomings by making NMT late-initializable: it can now be initialized after normal VM argument parsing, like all other parts of the VM. This greatly simplifies NMT initialization and makes it work automagically for every third party launcher, as well as within our gtests. The glaring problem with late-initializing NMT is the NMT malloc headers. If we rule out just always having them (unacceptable in terms of memory overhead), there is no safe way to determine, in os::free(), if an allocation came from before or after NMT initialization ran, and therefore what to do with its malloc headers. For a more extensive explanation, please see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and @zhengyu123 in the JBS comment section. The heart of this patch is a new way to track early, pre-NMT-init allocations. These are tracked via a lookup table. This was a suggestion by Kim and it worked out well. Changes in detail: - pre-NMT-init handling: - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init handling. They contain a small global lookup table managing C-heap blocks allocated in the pre-NMT-init phase. - `os::malloc()/os::realloc()/os::free()` defer to this code before doing anything else. - Please see the extensive comment block at the start of `nmtPreinit.hpp` explaining the details. - Changes to NMT: - Before, NMT initialization was spread over two phases, `initialize()` and `late_initialize()`. Those were merged into one and simplified - there is only one initialization now which happens after argument parsing. - Minor changes were needed for the `NMT_TrackingLevel` enum - to simplify code, I changed NMT_unknown to be numerically 0. A new comment block in `nmtCommon.hpp` now clearly specifies what's what, including allowed level state transitions. - New utility functions to translate tracking level from/to strings added to `NMTUtil` - NMT has never been able to handle virtual memory allocations before initialization, which is fine since os::reserve_memory() is not called before VM parses arguments. We now assert that. - All code outside the VM handling NMT initialization (eg. libjli) has been removed, as has the code testing it. - Gtests: - Some existing gtests had to be modified: before, they all changed global
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java line 191: > 189: installed[i]); > 190: } > 191: String[] retval = map.keySet().toArray(new String[0]); Looks like previously the code returns values, and now it will return keys, please recheck. - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Tue, 15 Jun 2021 12:34:50 GMT, Andrey Turbanov wrote: >> src/java.base/share/classes/java/security/Security.java line 656: >> >>> 654: return null; >>> 655: >>> 656: return candidates.toArray(new Provider[0]); >> >> `candidates.toArray(new Provider[candidates.size()]);` >> >> would use the fast path of the toArray() implementation. Performance >> probably doesn't matter much in a lot of this cases, but since its only a >> few characters more, its still worth considering IMO. >> >> The only places I would leave this out are on the HashTable and on the >> Vector collections in this PR, since calling one synchronized method is not >> the same as calling two - concurrency wise. >> >> (i am no reviewer) > > Actually it's not _the fast path_ - see > https://shipilev.net/blog/2016/arrays-wisdom-ancients/ oh I am sorry my mistake - I actually now remember reading the article. (It would be interesting to rerun the benchmark on modern JDKs since I would expect the gap to be smaller now) Please disregard my suggestion. - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. src/java.base/share/classes/java/security/Security.java line 656: > 654: return null; > 655: > 656: return candidates.toArray(new Provider[0]); `candidates.toArray(new Provider[candidates.size()]);` would use the fast path of the toArray() implementation. Performance probably doesn't matter much in a lot of this cases, but since its only a few characters more, its still worth considering IMO. The only places I would leave this out are on the HashTable and on the Vector collections in this PR, since calling one synchronized method is not the same as calling two - concurrency wise. (i am no reviewer) - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Tue, 15 Jun 2021 12:06:43 GMT, Michael Bien wrote: >> I found few places, where code initially perform `Object[] >> Colleciton.toArray()` call and then manually copy array into another array >> with required type. >> This PR cleanups such places to more shorter call `T[] >> Collection.toArray(T[])`. > > src/java.base/share/classes/java/security/Security.java line 656: > >> 654: return null; >> 655: >> 656: return candidates.toArray(new Provider[0]); > > `candidates.toArray(new Provider[candidates.size()]);` > > would use the fast path of the toArray() implementation. Performance probably > doesn't matter much in a lot of this cases, but since its only a few > characters more, its still worth considering IMO. > > The only places I would leave this out are on the HashTable and on the Vector > collections in this PR, since calling one synchronized method is not the same > as calling two - concurrency wise. > > (i am no reviewer) Actually it's not _the fast path_ - see https://shipilev.net/blog/2016/arrays-wisdom-ancients/ - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. I think the same simlification can be done for some classes affected by your previous PR https://github.com/openjdk/jdk/pull/4482, e.g. `HttpsClient`, lines 154-157 and 177-180 and `PKCS7`, so I'd wait for https://github.com/openjdk/jdk/pull/4482 and then add one more commit here. @turbanoff I've filed a ticket for this: https://bugs.openjdk.java.net/browse/JDK-8269130. Also I think you can integrate https://github.com/openjdk/jdk/pull/4482 - PR: https://git.openjdk.java.net/jdk/pull/4487
RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
I found few places, where code initially perform `Object[] Colleciton.toArray()` call and then manually copy array into another array with required type. This PR cleanups such places to more shorter call `T[] Collection.toArray(T[])`. - Commit messages: - Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying - Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying - Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying Changes: https://git.openjdk.java.net/jdk/pull/4487/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4487=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269130 Stats: 70 lines in 8 files changed: 0 ins; 54 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/4487.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4487/head:pull/4487 PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Mon, 26 Jul 2021 10:16:47 GMT, Lance Andersen wrote:
>> Hi,
>>
>> As discussed in the
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
>> thread, this is the revised patch to address the use of '.' and '..' within
>> Zip FS
>>
>> Zip FS needs to use "." and ".." as links to the current and parent
>> directories and cannot reliably support entries that have "." and ".." as
>> name elements. This patch updates Zip Fs to reject ZIP files that have
>> entries in the CEN that can't be used for files in a file system.
>>
>>
>> Mach5 tiers 1 through 3 have been run without any errors encountered .
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add missing Copyright header and address minor comments
> zipfs.getPath("./Hello.txt"),
> zipfs.getPath("../../../Hello.txt"),
> zipfs.getPath("../Hello.txt")};
>
>
> In other words, the `ZipFileSystem` doesn't end up creating a zip file which
> is then rejected by `ZipFileSystem` when creating a new filesystem using
> `Files.newFileSystem`. That's a good thing.
Right, it's always existing behavior and matches the behavior of the platform
file system.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Mon, 26 Jul 2021 09:52:09 GMT, Jaikiran Pai wrote:
> This change looks fine to me. I was unsure how the writing/creating entries
> with `.` or `..` with `ZipFileSystem` would behave in context of this change,
> so I gave this a try locally with the changes from this PR:
>
> ```
> try (FileSystem zipfs = FileSystems.newFileSystem(ZIPFILE)) {
> final Path[] paths = new Path[]{
> zipfs.getPath("./Hello.txt"),
> zipfs.getPath("../../../Hello.txt"),
> zipfs.getPath("../Hello.txt")};
> for (int i = 0; i < paths.length; i++) {
> try (OutputStream os = Files.newOutputStream(paths[i])) {
> os.write(("Hello " + i).getBytes(StandardCharsets.UTF_8));
> }
> }
> }
> ```
>
> This code runs fine and it ends up creating (just one) CEN entry for
> `Hello.txt`:
>
> ```
> JTwork/scratch/zipfsDotDotTest.zip
> Length DateTimeName
> - -- -
> 7 07-26-2021 15:07 Hello.txt
> ```
>
> In other words, the `ZipFileSystem` doesn't end up creating a zip file which
> is then rejected by `ZipFileSystem` when creating a new filesystem using
> `Files.newFileSystem`. That's a good thing.
With the exception of creating the Inodes table, Zip FS always calls
ZipPath::getResolvedPath for access to Zip entries. So there is no issues with
the creation and access of entries in the case above
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Mon, 26 Jul 2021 07:30:12 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add missing Copyright header and address minor comments
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1573:
>
>> 1571: }
>> 1572: IndexNode inode = new IndexNode(cen, pos, nlen);
>> 1573: if(hasDotOrDotDot(inode.name)) {
>
> Minor nit, missing space in "if(".
fixed
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1602:
>
>> 1600: // Inode.name always includes "/" in path[0]
>> 1601: if (path.length == 1) {
>> 1602: return false;
>
> It may be useful to add "assert path[0] == '/';" at the start of this method.
I added it per your suggestion, though IndexNode(byte[] cen, int pos, int nlen)
which is only used by initCen() will already guarantee the leading "/" is there
> test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 1:
>
>> 1: import org.testng.annotations.DataProvider;
>
> Missing copyright header.
Geez, how did I miss that. Added
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add missing Copyright header and address minor comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/68af64c4..2265ffe1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4900=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4900=00-01 Stats: 25 lines in 2 files changed: 24 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 25 Jul 2021 21:56:10 GMT, Lance Andersen wrote:
> Hi,
>
> As discussed in the
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
> thread, this is the revised patch to address the use of '.' and '..' within
> Zip FS
>
> Zip FS needs to use "." and ".." as links to the current and parent
> directories and cannot reliably support entries that have "." and ".." as
> name elements. This patch updates Zip Fs to reject ZIP files that have
> entries in the CEN that can't be used for files in a file system.
>
>
> Mach5 tiers 1 through 3 have been run without any errors encountered .
>
> Best,
> Lance
This change looks fine to me. I was unsure how the writing/creating entries
with `.` or `..` with `ZipFileSystem` would behave in context of this change,
so I gave this a try locally with the changes from this PR:
try (FileSystem zipfs = FileSystems.newFileSystem(ZIPFILE)) {
final Path[] paths = new Path[]{
zipfs.getPath("./Hello.txt"),
zipfs.getPath("../../../Hello.txt"),
zipfs.getPath("../Hello.txt")};
for (int i = 0; i < paths.length; i++) {
try (OutputStream os = Files.newOutputStream(paths[i])) {
os.write(("Hello " + i).getBytes(StandardCharsets.UTF_8));
}
}
}
This code runs fine and it ends up creating (just one) CEN entry for
`Hello.txt`:
JTwork/scratch/zipfsDotDotTest.zip
Length DateTimeName
- -- -
7 07-26-2021 15:07 Hello.txt
In other words, the `ZipFileSystem` doesn't end up creating a zip file which is
then rejected by `ZipFileSystem` when creating a new filesystem using
`Files.newFileSystem`. That's a good thing.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available [v7]
> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 I've found
> out, that in a few of JDK core classes, e.g. in `j.l.Class` expressions like
> `baseName.replace('.', '/') + '/' + name` are not compiled into
> `invokedynamic`-based code, but into one using `StringBuilder`. This happens
> due to some bootstraping issues.
>
> However the code like
>
> private String getSimpleName0() {
> if (isArray()) {
> return getComponentType().getSimpleName() + "[]";
> }
> //...
> }
>
> can be improved via replacement of `+` with `String.concat()` call.
>
> I've used this benchmark to measure impact:
>
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ClassMethodsBenchmark {
> private final Class arrayClass = Object[].class;
> private Method descriptorString;
>
> @Setup
> public void setUp() throws NoSuchMethodException {
> //fore some reason compiler doesn't allow me to call
> Class.descriptorString() directly
> descriptorString = Class.class.getDeclaredMethod("descriptorString");
> }
>
> @Benchmark
> public Object descriptorString() throws Exception {
> return descriptorString.invoke(arrayClass);
> }
>
> @Benchmark
> public Object typeName() {
> return arrayClass.getTypeName();
> }
> }
>
> and got those results
>
>Mode Cnt Score
> Error Units
> descriptorString avgt 6091.480 ±
> 1.839 ns/op
> descriptorString:·gc.alloc.rate.norm avgt 60 404.008 ±
> 4.033B/op
> descriptorString:·gc.churn.G1_Eden_Space avgt 60 2791.866 ±
> 181.589 MB/sec
> descriptorString:·gc.churn.G1_Eden_Space.norm avgt 60 401.702 ±
> 26.047B/op
> descriptorString:·gc.churn.G1_Survivor_Space avgt 60 0.003 ±
> 0.001 MB/sec
> descriptorString:·gc.churn.G1_Survivor_Space.norm avgt 60≈ 10⁻³
> B/op
> descriptorString:·gc.count avgt 60 205.000
> counts
> descriptorString:·gc.time avgt 60 216.000
> ms
>
> patched
>Mode Cnt Score
> Error Units
> descriptorString avgt 6065.016 ±
> 3.446 ns/op
> descriptorString:·gc.alloc.rateavgt 60 2844.240 ±
> 115.719 MB/sec
> descriptorString:·gc.alloc.rate.norm avgt 60 288.006 ±
> 0.001B/op
> descriptorString:·gc.churn.G1_Eden_Space avgt 60 2832.996 ±
> 206.939 MB/sec
> descriptorString:·gc.churn.G1_Eden_Space.norm avgt 60 286.955 ±
> 17.853B/op
> descriptorString:·gc.churn.G1_Survivor_Space avgt 60 0.003 ±
> 0.001 MB/sec
> descriptorString:·gc.churn.G1_Survivor_Space.norm avgt 60≈ 10⁻³
> B/op
> descriptorString:·gc.count avgt 60 208.000
> counts
> descriptorString:·gc.time avgt 60 228.000
> ms
> -
> typeName avgt 6034.273 ±
> 0.480 ns/op
> typeName:·gc.alloc.rateavgt 60 3265.356 ±
> 45.113 MB/sec
> typeName:·gc.alloc.rate.norm avgt 60 176.004 ±
> 0.001B/op
> typeName:·gc.churn.G1_Eden_Space avgt 60 3268.454 ±
> 134.458 MB/sec
> typeName:·gc.churn.G1_Eden_Space.norm avgt 60 176.109 ±
> 6.595B/op
> typeName:·gc.churn.G1_Survivor_Space avgt 60 0.003 ±
> 0.001 MB/sec
> typeName:·gc.churn.G1_Survivor_Space.norm avgt 60≈ 10⁻⁴
> B/op
> typeName:·gc.count avgt 60 240.000
> counts
> typeName:·gc.time avgt 60 255.000
> ms
>
> patched
>
> typeName avgt 6015.787 ±
> 0.214 ns/op
> typeName:·gc.alloc.rateavgt 60 2577.554 ±
> 32.339 MB/sec
> typeName:·gc.alloc.rate.norm avgt 6064.001 ±
> 0.001B/op
> typeName:·gc.churn.G1_Eden_Space avgt 60 2574.039 ±
> 147.774 MB/sec
> typeName:·gc.churn.G1_Eden_Space.norm avgt 6063.895 ±
> 3.511B/op
> typeName:·gc.churn.G1_Survivor_Space avgt 60 0.003 ±
> 0.001 MB/sec
> typeName:·gc.churn.G1_Survivor_Space.norm avgt 60≈ 10⁻⁴
> B/op
> typeName:·gc.count avgt 60 189.000
> counts
> typeName:·gc.time avgt 60 199.000
> ms
>
> I think this patch is likely to improve
Re: RFR: 8263561: Re-examine uses of LinkedList [v5]
> 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 - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4304=04 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v11]
> There is a few JDK classes duplicating the contents of Long.hashCode() for > hash code calculation. They should explicitly delegate to Long.hashCode(). Сергей Цыпанов 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 13 additional commits since the last revision: - Merge branch 'master' into 8268113 - 8270160 Revert changes in BitSet.hashCode - Merge branch 'master' into 8268113 - 8270160 Revert changes in BitSet.hashCode - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/441e382b...bd762b7d - Changes: - all: https://git.openjdk.java.net/jdk/pull/4309/files - new: https://git.openjdk.java.net/jdk/pull/4309/files/1d619c73..bd762b7d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4309=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4309=09-10 Stats: 7986 lines in 302 files changed: 5011 ins; 1046 del; 1929 mod Patch: https://git.openjdk.java.net/jdk/pull/4309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4309/head:pull/4309 PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt() [v4]
> `AbstractStringBuilder.charAt(int)` does bounds check before calling
> `charAt()` (for non-latin Strings):
>
> @Override
> public char charAt(int index) {
> checkIndex(index, count);
> if (isLatin1()) {
> return (char)(value[index] & 0xff);
> }
> return StringUTF16.charAt(value, index);
> }
>
> This can be improved by removing bounds check from ASB.charAt() in favour of
> one in String*.charAt(). This gives slight improvement:
>
> before
> Benchmark Mode Cnt Score Error Units
> StringBuilderCharAtBenchmark.latin avgt 50 2,827 ± 0,024 ns/op
> StringBuilderCharAtBenchmark.utfavgt 50 2,985 ± 0,020 ns/op
>
> after
> Benchmark Mode Cnt Score Error Units
> StringBuilderCharAtBenchmark.latin avgt 50 2,434 ± 0,004 ns/op
> StringBuilderCharAtBenchmark.utfavgt 50 2,631 ± 0,004 ns/op
Сергей Цыпанов has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains three commits:
- Merge branch 'master' into 8270160
- Merge branch 'master' into 8270160
# Conflicts:
#src/java.base/share/classes/java/lang/StringLatin1.java
- 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()
-
Changes: https://git.openjdk.java.net/jdk/pull/4738/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4738=03
Stats: 6 lines in 2 files changed: 0 ins; 3 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/4738.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4738/head:pull/4738
PR: https://git.openjdk.java.net/jdk/pull/4738
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 25 Jul 2021 21:56:10 GMT, Lance Andersen wrote:
> Hi,
>
> As discussed in the
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
> thread, this is the revised patch to address the use of '.' and '..' within
> Zip FS
>
> Zip FS needs to use "." and ".." as links to the current and parent
> directories and cannot reliably support entries that have "." and ".." as
> name elements. This patch updates Zip Fs to reject ZIP files that have
> entries in the CEN that can't be used for files in a file system.
>
>
> Mach5 tiers 1 through 3 have been run without any errors encountered .
>
> Best,
> Lance
This is behavior change (to reject zip/JAR files) so a CSR will be required.
I've also added a label to the JBS issue to remind us to add a release note.
I think it would be good for @jaikiran to review too.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1573:
> 1571: }
> 1572: IndexNode inode = new IndexNode(cen, pos, nlen);
> 1573: if(hasDotOrDotDot(inode.name)) {
Minor nit, missing space in "if(".
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1602:
> 1600: // Inode.name always includes "/" in path[0]
> 1601: if (path.length == 1) {
> 1602: return false;
It may be useful to add "assert path[0] == '/';" at the start of this method.
test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 1:
> 1: import org.testng.annotations.DataProvider;
Missing copyright header.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
