Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase #29

Closed
wants to merge 3 commits into from

Conversation

@doom369
Copy link

@doom369 doom369 commented Sep 6, 2020

isEmpty is faster + has less byte code + easier to read. Benchmarks could be found here.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8252999: Cleanup: Replace .equals("") with .isEmpty() throughout the codebase ⚠️ Title mismatch between PR and JBS.

Download

$ git fetch https://git.openjdk.java.net/jdk pull/29/head:pull/29
$ git checkout pull/29

@bridgekeeper bridgekeeper bot added the oca label Sep 6, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 6, 2020

Hi @doom369, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user doom369" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 6, 2020

@doom369 The following labels will be automatically applied to this pull request: awt build hotspot i18n javadoc jdk jmx net nio security serviceability swing.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@doom369
Copy link
Author

@doom369 doom369 commented Sep 6, 2020

/signed

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 6, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@thatsIch
Copy link

@thatsIch thatsIch commented Sep 6, 2020

@doom369 jcheck requires an associated issue

@doom369
Copy link
Author

@doom369 doom369 commented Sep 9, 2020

@mrserb hello. Thanks for the review. Any further actions required from me?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 9, 2020

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. 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).

@doom369 doom369 changed the title Improvement: replace all String.equals("") usages with String.isEmpty() 8252999: replace all String.equals("") usages with String.isEmpty() Sep 10, 2020
@openjdk openjdk bot added the rfr label Sep 10, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 10, 2020

Webrevs

@doom369 doom369 changed the title 8252999: replace all String.equals("") usages with String.isEmpty() 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase Sep 10, 2020
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 10, 2020

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.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 10, 2020

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.

@doom369
Copy link
Author

@doom369 doom369 commented Sep 10, 2020

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.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 10, 2020

@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?

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 10, 2020

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. :(

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Sep 10, 2020

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.

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Sep 10, 2020

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).

Please note that we can not consider any information on external websites for the review, so please add any relevant information to the PR body instead.

Also, what testing has been done for these changes?

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Sep 10, 2020

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.

Copy link
Contributor

@prrace prrace left a comment

  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.

@doom369
Copy link
Author

@doom369 doom369 commented Sep 11, 2020

Ok, sorry for the distraction.

@doom369 doom369 closed this Sep 11, 2020
@bradfordwetmore
Copy link
Contributor

@bradfordwetmore bradfordwetmore commented Sep 11, 2020

Our local Santuario maintainer says:

In general, changes to Apache Santuario should also be made at Apache so we stay in sync.

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Sep 18, 2020

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.

@doom369
Copy link
Author

@doom369 doom369 commented Sep 18, 2020

@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.

theRealELiu added a commit to theRealELiu/jdk that referenced this issue Nov 13, 2020
This patch is a supplement for
https://bugs.openjdk.java.net/browse/JDK-8248830.

With the implementation of rotate node in IR, this patch:

1. canonicalizes RotateLeft into RotateRight when shift is a constant,
   so that GVN could identify the pre-existing node better.
2. implements scalar rotate match rules and removes the original
   combinations of Or and Shifts on AArch64.

This patch doesn't implement vector rotate due to the lack of
corresponding vector instructions on AArch64.

Test case below is an explanation for this patch.

        public static int test(int i) {
            int a =  (i >>> 29) | (i << -29);
            int b = i << 3;
            int c = i >>> -3;
            int d = b | c;
            return a ^ d;
        }

Before:

        lsl     w12, w1, openjdk#3
        lsr     w10, w1, openjdk#29
        add     w11, w10, w12
        orr     w12, w12, w10
        eor     w0, w11, w12

After:

        ror     w10, w1, openjdk#29
        eor     w0, w10, w10

Tested jtreg TestRotate.java, hotspot::hotspot_all_no_apps,
jdk::jdk_core, langtools::tier1.

Change-Id: Id7d00935945f1697247fff7041b0707107862786
@mlbridge mlbridge bot mentioned this pull request Nov 16, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment