Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Fri, 18 Sep 2020 02:48:15 GMT, Stuart Marks wrote: >> Our local Santuario maintainer says: >> >> In general, changes to Apache Santuario should also be made at Apache so we >> stay in sync. > > Hi @doom369, I hope we didn't end up wasting too much of your time with this. > I wanted to respond to a comment you made > earlier in this PR, >> I have in mind dozens of improvements all over the code like this one. > > It's hard to see, but as you discovered, the JDK has different groups of > people maintaining different areas, and > sometimes there are hidden constraints on those different areas, for example, > to avoid divergence with upstream source > bases. And as you discovered, sometimes those source bases might need to > maintain compatibility with an older JDK ... > so we don't want to update this code even though it's IN the JDK. Those kind > of constraints generally don't apply to > code in the java.base module, since there's nothing upstream of it, and by > definition it cannot depend on anything > outside the JDK. Generally -- though there are exceptions -- we're more > receptive to keeping stuff in java.base (and > sometimes related modules close to the core) on the latest and greatest > stuff. There are some constraints, however. > There are some things we can't use too early during initialization of the > JDK, such as lambdas. Also, there is some > code lurking around that is sometimes executed by the boot JDK, which is > typically one release behind. (This is > definitely the case for tools like javac, but I think it might also apply to > some things in base.) Anyway, if you'd > like to pursue some of these improvements, drop a note to > core-libs-dev@openjdk and we can talk about it. Thanks. @stuart-marks thanks. Sure, I understand that maintaining OpenJDK is not a simple task. I thought as change is super simple without any side effects it can go through. But I was wrong. That's fine. I'll focus on `java.base` when I have some free time. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Fri, 11 Sep 2020 15:17:58 GMT, Bradford Wetmore wrote: >> 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. Hi @doom369, I hope we didn't end up wasting too much of your time with this. I wanted to respond to a comment you made earlier in this PR, > I have in mind dozens of improvements all over the code like this one. It's hard to see, but as you discovered, the JDK has different groups of people maintaining different areas, and sometimes there are hidden constraints on those different areas, for example, to avoid divergence with upstream source bases. And as you discovered, sometimes those source bases might need to maintain compatibility with an older JDK ... so we don't want to update this code even though it's IN the JDK. Those kind of constraints generally don't apply to code in the java.base module, since there's nothing upstream of it, and by definition it cannot depend on anything outside the JDK. Generally -- though there are exceptions -- we're more receptive to keeping stuff in java.base (and sometimes related modules close to the core) on the latest and greatest stuff. There are some constraints, however. There are some things we can't use too early during initialization of the JDK, such as lambdas. Also, there is some code lurking around that is sometimes executed by the boot JDK, which is typically one release behind. (This is definitely the case for tools like javac, but I think it might also apply to some things in base.) Anyway, if you'd like to pursue some of these improvements, drop a note to core-libs-dev@openjdk and we can talk about it. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/29
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: 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: 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). 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. - Changes requested by prr (Reviewer). 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 12:04:48 GMT, Dmitriy Dumanskiy wrote: > I have in mind dozens of improvements all over the code like this one. That sounds scary. Broad updates like these cause unnecessary churn in the codebase, and can make merging and back porting harder. Changes should be discussed ahead of time with the appropriate teams. - 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 15:50:39 GMT, Alan Bateman wrote: >> 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to >> post, but even if you are a reply-all will >> be delayed due to all of the mails being held up for moderator approval due >> to: >> " Too many recipients to the message" >> >> I have a longer email coming once it gets through the moderator approval >> delay. :( > > Patches that do global replace are always awkward. In this case, there are > 150 files changed and most seem to be tests > or changes to tools used in the build (includes > src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from > the patch then it leaves just 43 files, many of which are from 3rd party code > that can also be dropped. This should > reduce the patch down to a manageable 24 or so files that should be trivial > to review. Since one of the motivations for this change is micro benchmark performance, please add a benchmark to the JDKs micro benchmark suite as well. (see e.g. https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/lang). Also, what testing has been done for these changes? - 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 13:53:10 GMT, David Holmes wrote: >> @dholmes-ora raises a good point. Hopefully there is a balance point between >> a dozen different bugs / pull requests >> each targeting one area and one bug / PR targeting a dozen separate areas. I >> don't have a good general rule to suggest. >> Maybe @AlanBateman or @jddarcy can offer some advice? > > 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to > post, but even if you are a reply-all will > be delayed due to all of the mails being held up for moderator approval due > to: > " Too many recipients to the message" > > I have a longer email coming once it gets through the moderator approval > delay. :( Patches that do global replace are always awkward. In this case, there are 150 files changed and most seem to be tests or changes to tools used in the build (includes src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from the patch then it leaves just 43 files, many of which are from 3rd party code that can also be dropped. This should reduce the patch down to a manageable 24 or so files that should be trivial to review. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On 10/09/2020 10:07 pm, Dmitriy Dumanskiy wrote: On Thu, 10 Sep 2020 11:21:28 GMT, David Holmes wrote: The code in java.base was updated to use String::isEmpty in JDK 12 (JDK-8215281). There was follow-up in JDK 13 to do the same in the java.desktop module (JDK-8223237). Changing the remaining usages make sense although I see that more more than half are in tests. It would be good to hear from security-dev on the changes to the Apache Santuario code (in java.xml.crypto module) in case it would be better to contribute those upstream instead. Ditto for the Apache Xalan code (in the java.xml module) but it may be significantly forked already that it doesn't matter. I assume you can use JDK-8215014 rather than creating a new issue. This should be broken up to deal with the files in different functional areas under different bugids and PRs. Otherwise the cross-posting to so many lists is prohibitive. Files in different areas need to be reviewed by different teams. Thank you. I have in mind dozens of improvements all over the code like this one. I already did some further review and in most cases, those tiny changes go trough all codebase. I can create PR for every separate module, but in general, it would multiply x5 the number of PRs in total. If you think it's a better way to go, no problem, I can do that. Find a reasonable middle ground. You have around 14 mailing lists cc'd here, for changes on 150 files. It is very unlikely anyone will review all 150, and files in different areas are, by convention, reviewed by Reviewers for those areas. Even if people only look at a subset of files, communicating that to you effectively so you can figure out when all 150 have been reviewed, is not very practical. My initial breakdown would be: - build - desktop (awt/swing/2d) - serviceability/jmx (the SA and JVMTI changes) - security - core-libs Also note that while the original PR email went out to 14 lists, most people trying to reply-all won't be able to as they don't subscribe to all 14 lists, so the review threads will get very fragmented. Cheers, David - - 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 12:18:51 GMT, Kevin Rushforth wrote: >> This should be broken up to deal with the files in different functional >> areas under different bugids and PRs. Otherwise >> the cross-posting to so many lists is prohibitive. Files in different areas >> need to be reviewed by different teams. >> Thank you. > > @dholmes-ora raises a good point. Hopefully there is a balance point between > a dozen different bugs / pull requests > each targeting one area and one bug / PR targeting a dozen separate areas. I > don't have a good general rule to suggest. > Maybe @AlanBateman or @jddarcy can offer some advice? 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to post, but even if you are a reply-all will be delayed due to all of the mails being held up for moderator approval due to: " Too many recipients to the message" I have a longer email coming once it gets through the moderator approval delay. :( - 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 11:21:28 GMT, David Holmes wrote: >> The code in java.base was updated to use String::isEmpty in JDK 12 >> (JDK-8215281). There was follow-up in JDK 13 to do >> the same in the java.desktop module (JDK-8223237). Changing the remaining >> usages make sense although I see that more >> more than half are in tests. It would be good to hear from security-dev on >> the changes to the Apache Santuario code >> (in java.xml.crypto module) in case it would be better to contribute those >> upstream instead. Ditto for the Apache Xalan >> code (in the java.xml module) but it may be significantly forked already >> that it doesn't matter. I assume you can use >> JDK-8215014 rather than creating a new issue. > > This should be broken up to deal with the files in different functional areas > under different bugids and PRs. Otherwise > the cross-posting to so many lists is prohibitive. Files in different areas > need to be reviewed by different teams. > Thank you. @dholmes-ora raises a good point. Hopefully there is a balance point between a dozen different bugs / pull requests each targeting one area and one bug / PR targeting a dozen separate areas. I don't have a good general rule to suggest. Maybe @AlanBateman or @jddarcy can offer some advice? - 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 11:21:28 GMT, David Holmes wrote: >> The code in java.base was updated to use String::isEmpty in JDK 12 >> (JDK-8215281). There was follow-up in JDK 13 to do >> the same in the java.desktop module (JDK-8223237). Changing the remaining >> usages make sense although I see that more >> more than half are in tests. It would be good to hear from security-dev on >> the changes to the Apache Santuario code >> (in java.xml.crypto module) in case it would be better to contribute those >> upstream instead. Ditto for the Apache Xalan >> code (in the java.xml module) but it may be significantly forked already >> that it doesn't matter. I assume you can use >> JDK-8215014 rather than creating a new issue. > > This should be broken up to deal with the files in different functional areas > under different bugids and PRs. Otherwise > the cross-posting to so many lists is prohibitive. Files in different areas > need to be reviewed by different teams. > Thank you. I have in mind dozens of improvements all over the code like this one. I already did some further review and in most cases, those tiny changes go trough all codebase. I can create PR for every separate module, but in general, it would multiply x5 the number of PRs in total. If you think it's a better way to go, no problem, I can do that. - 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 10:40:15 GMT, Alan Bateman wrote: >> @kevinrushforth thanks. Done. >> >> Similar issues: >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 >> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 >> >> could be joined somehow? > > The code in java.base was updated to use String::isEmpty in JDK 12 > (JDK-8215281). There was follow-up in JDK 13 to do > the same in the java.desktop module (JDK-8223237). Changing the remaining > usages make sense although I see that more > more than half are in tests. It would be good to hear from security-dev on > the changes to the Apache Santuario code > (in java.xml.crypto module) in case it would be better to contribute those > upstream instead. Ditto for the Apache Xalan > code (in the java.xml module) but it may be significantly forked already that > it doesn't matter. I assume you can use > JDK-8215014 rather than creating a new issue. This should be broken up to deal with the files in different functional areas under different bugids and PRs. Otherwise the cross-posting to so many lists is prohibitive. Files in different areas need to be reviewed by different teams. Thank you. - 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 08:47:35 GMT, Dmitriy Dumanskiy wrote: >> Before this Enhancement can be formally reviewed, you will need a JBS bug >> ID. If you are already working with a >> Committer or Reviewer in the `jdk` project who has agreed to sponsor your >> change, they can file the Enhancement >> request. Otherwise, you can file it using the web interface at >> [bugreport.java.com](https://bugreport.java.com/). Once >> you have a JBS bug ID, you need to edit the PR title to include that bug ID >> (without the `JDK-`) replacing >> "Improvement". Since this PR cuts across many functional areas (each gray >> label represents a functional area), you >> should expect a longer review process, since someone from each functional >> area will need to look at the changes in >> their area (like @mrserb started to do for the `2d` area). > > @kevinrushforth thanks. Done. > > Similar issues: > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 > > could be joined somehow? The code in java.base was updated to use String::isEmpty in JDK 12 (JDK-8215281). There was follow-up in JDK 13 to do the same in the java.desktop module (JDK-8223237). Changing the remaining usages make sense although I see that more more than half are in tests. It would be good to hear from security-dev on the changes to the Apache Santuario code (in java.xml.crypto module) in case it would be better to contribute those upstream instead. Ditto for the Apache Xalan code (in the java.xml module) but it may be significantly forked already that it doesn't matter. I assume you can use JDK-8215014 rather than creating a new issue. - PR: https://git.openjdk.java.net/jdk/pull/29
