Withdrawn: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy wrote: > **isEmpty** is faster + has less byte code + easier to read. Benchmarks could > be found > > [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 23:57:38 GMT, Phil Race wrote: >> **isEmpty** is faster + has less byte code + easier to read. Benchmarks >> could be found >> >> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416). > > 1) This is un-necessary churn. > 2) I can't even be sure I am finding the ones in my area because there's so > much here > 3) The ones I can find have no need of whatever performance improvement this > might bring. > I think this whole PR should be withdrawn, and there may an attempt at > measuring the benefits of this for the various > cases before submitting in appropriate smaller chunks. But I'll tell you now > I don't see a need for the test updates > you are making. Ok, sorry for the distraction. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8245194: Unix domain socket channel implementation [v2]
> Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more > iterations before integrating. > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, >>> I added a method to WindowsPath which returns the path in the expected >>> UTF-8 encoding as a byte[]. Let me know what you >>> think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the >> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about >> encoding a relative path when the resolved >> path (before encoding) is > 247 chars. The documentation on the MS site >> isn't very completely and I think there are a >> number points that require clarification to fully understand how this will >> work with relative, directly relative and >> drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That > would leave WindowsPath unchanged. >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a >> method to AbstractFileSystemProvider instead. That is the base class the >> platform providers and would be a better place >> to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to > be useful by other components. >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its >> fromUid and fromGid method to be public. This would mean that the patch >> shouldn't need to add UnixUserGroup (the main >> issue is that class is that it means we end up with two classes with static >> factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >> Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: Made some changes relating to selection of the local directory where automatically bound server sockets are created. After this change it is no longer necessary to specify the location in individual tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/6d164c91..4d08c15a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=00-01 Stats: 119 lines in 10 files changed: 89 ins; 18 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation
On Mon, 7 Sep 2020 12:05:07 GMT, Michael McMahon wrote: > Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more > iterations before integrating. > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, >>> I added a method to WindowsPath which returns the path in the expected >>> UTF-8 encoding as a byte[]. Let me know what you >>> think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the >> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about >> encoding a relative path when the resolved >> path (before encoding) is > 247 chars. The documentation on the MS site >> isn't very completely and I think there are a >> number points that require clarification to fully understand how this will >> work with relative, directly relative and >> drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That > would leave WindowsPath unchanged. >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a >> method to AbstractFileSystemProvider instead. That is the base class the >> platform providers and would be a better place >> to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to > be useful by other components. >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its >> fromUid and fromGid method to be public. This would mean that the patch >> shouldn't need to add UnixUserGroup (the main >> issue is that class is that it means we end up with two classes with static >> factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >> > _Mailing list message from [Alan Bateman](mailto:alan.bate...@oracle.com) on > [nio-dev](mailto:nio-...@openjdk.java.net):_ > On 07/09/2020 13:14, Michael McMahon wrote: > > > > In the same area, the new PathUtil is a bit inconsistent with the > > > existing provider code. One suggestion is to add a > > > method to AbstractFileSystemProvider instead. That is the base class the > > > platform providers and would be a better place > > > to get the file path in bytes. > > > > > > Okay, I gave the method a name that is specific to Unix domain sockets > > because this UTF-8 strangeness is not likely to > > be useful by other components. > > The file system provider shouldn't know anything about Unix domain > sockets so I prefer not to have "UnixDomain" in the name of this method. > I also would prefer if null is an exception so that it is consistent > with the other methods. > I'm not sure what to call the method then. It returns a UTF-8 string converted to bytes on Windows and the output of getByteArrayForSysCalls on Unix. It could be called getByteArrayUTF8 on Windows, but that would not be right on Unix. Since sockets are file system entities, why not have socket in the method name? What about getByteArrayForSocket() ? > I think it would be useful to also summarize how the bind/connect works > on Windows. For example, suppose the working directory is 240 characters > and the UnixDomainSocketAddress is to a simple relative path of 20 > characters. If I understand correctly, the proposal will encode the > simple relative path (not the resolved absolute path) to bytes using > UTF-8 so it will probably be 20+ bytes in this case. Right > This hould be okay > but inconsistent with the rest of? file system implementation which will > use the long form. Also if the name is long then it won't use the long > path prefix (\?) but instead rely on failure because the resulting > sequence of bytes when encoded is > 100, is that correct? > Yes, that is how it is working currently. We check the length in native code just before calling bind(). Whether the long path prefix is present or not won't matter because the path will not be used once it is longer than ~100 bytes. Michael. > -Alan. - PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Fri, 11 Sep 2020 07:15:26 GMT, Dmitriy Dumanskiy wrote: >> 1) This is un-necessary churn. >> 2) I can't even be sure I am finding the ones in my area because there's so >> much here >> 3) The ones I can find have no need of whatever performance improvement this >> might bring. >> I think this whole PR should be withdrawn, and there may an attempt at >> measuring the benefits of this for the various >> cases before submitting in appropriate smaller chunks. But I'll tell you now >> I don't see a need for the test updates >> you are making. > > Ok, sorry for the distraction. Our local Santuario maintainer says: In general, changes to Apache Santuario should also be made at Apache so we stay in sync. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher. [v3]
> Continuing the review with a PR... > > 8252374: Add a new factory method to concatenate a sequence > of BodyPublisher instances into a single publisher. > https://bugs.openjdk.java.net/browse/JDK-8252374 > > > Draft CSR: > https://bugs.openjdk.java.net/browse/JDK-8252382 Daniel Fuchs 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 three additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into concat-bs-8252374 - Merge remote-tracking branch 'origin/master' into concat-bs-8252374 - 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher. - Changes: - all: https://git.openjdk.java.net/jdk/pull/57/files - new: https://git.openjdk.java.net/jdk/pull/57/files/befc3ab9..d76633b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=57&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=57&range=01-02 Stats: 7800 lines in 193 files changed: 3325 ins; 3055 del; 1420 mod Patch: https://git.openjdk.java.net/jdk/pull/57.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/57/head:pull/57 PR: https://git.openjdk.java.net/jdk/pull/57
