Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-13 Thread Claes Redestad 

On 2018-12-13 14:06, Daniel Fuchs wrote:

Looks good Claes!


Thanks!


I eyeballed the rest of the patch and found
nothing wrong - but with a patch this size
it would be easy to miss something.


Yes, I've gone through it a couple of times now
to be sure.



Were you able to measure any improvement after
patching?


There's a tiny reduction in bytecode executed
in initPhase1, but on any real measures any
improvement is in the noise.

/Claes



best regards,

-- daniel

Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-13 Thread Daniel Fuchs 

Looks good Claes!

I eyeballed the rest of the patch and found
nothing wrong - but with a patch this size
it would be easy to miss something.

Were you able to measure any improvement after
patching?

best regards,

-- daniel

On 12/12/2018 17:06, Claes Redestad wrote:



On 2018-12-12 17:54, Daniel Fuchs wrote:

Hi Claes,

It might read even better if things like:

+    resultString = !specarg.isEmpty() ? specarg.intern(): opt;

were changed into:

+    resultString = specarg.isEmpty() ? opt : specarg.intern();

best regards,


I only found this pattern in this file, so incremental
diff will look like the below. I will update in place due hugeness of
webrev.

Thanks!

/Claes

diff -r 732b03e40349 
src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java 
Wed Dec 12 17:41:46 2018 +0100
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java 
Wed Dec 12 18:03:57 2018 +0100

@@ -641,10 +641,10 @@
  String specarg = spec.substring(sidx);
  switch (specop) {
  case '.':  // terminate the option sequence
-    resultString = !specarg.isEmpty() ? 
specarg.intern(): opt;
+    resultString = specarg.isEmpty() ? opt : 
specarg.intern();

  break doArgs;
  case '?':  // abort the option sequence
-    resultString = !specarg.isEmpty() ? 
specarg.intern(): arg;
+    resultString = specarg.isEmpty() ? arg : 
specarg.intern();

  isError = true;
  break eachSpec;
  case '@':  // change the effective opt name
@@ -655,7 +655,7 @@
  val = "";
  break;
  case '!':  // negation option
-    String negopt = !specarg.isEmpty() ? 
specarg.intern(): opt;
+    String negopt = specarg.isEmpty() ? opt : 
specarg.intern();

  properties.remove(negopt);
  properties.put(negopt, null);  // leave 
placeholder

  didAction = true;

Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-12 Thread Claes Redestad 




On 2018-12-12 17:54, Daniel Fuchs wrote:

Hi Claes,

It might read even better if things like:

+    resultString = !specarg.isEmpty() ? specarg.intern(): opt;

were changed into:

+    resultString = specarg.isEmpty() ? opt : specarg.intern();

best regards,


I only found this pattern in this file, so incremental
diff will look like the below. I will update in place due hugeness of
webrev.

Thanks!

/Claes

diff -r 732b03e40349 
src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java 
Wed Dec 12 17:41:46 2018 +0100
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/Driver.java 
Wed Dec 12 18:03:57 2018 +0100

@@ -641,10 +641,10 @@
 String specarg = spec.substring(sidx);
 switch (specop) {
 case '.':  // terminate the option sequence
-resultString = !specarg.isEmpty() ? 
specarg.intern(): opt;
+resultString = specarg.isEmpty() ? opt : 
specarg.intern();

 break doArgs;
 case '?':  // abort the option sequence
-resultString = !specarg.isEmpty() ? 
specarg.intern(): arg;
+resultString = specarg.isEmpty() ? arg : 
specarg.intern();

 isError = true;
 break eachSpec;
 case '@':  // change the effective opt name
@@ -655,7 +655,7 @@
 val = "";
 break;
 case '!':  // negation option
-String negopt = !specarg.isEmpty() ? 
specarg.intern(): opt;
+String negopt = specarg.isEmpty() ? opt : 
specarg.intern();

 properties.remove(negopt);
 properties.put(negopt, null);  // leave 
placeholder

 didAction = true;

Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-12 Thread Claes Redestad 




On 2018-12-12 17:56, Alan Bateman wrote:
In Checks.java, the parameter change from CharSequence to String means 
that "cs" needs to be renamed.


Changed to 'str'

/Claes

Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-12 Thread Alan Bateman 

On 12/12/2018 16:53, Claes Redestad wrote:

Hi,

please review this patch to use String.isEmpty when applicable:

Webrev: http://cr.openjdk.java.net/~redestad/8215281/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215281

Why?
- It reads better :-)
- Better startup/warmup due fewer method invocations
- Better peak performance: String.isEmpty is ~1.2x faster than
  String.length

Most changes are simple search and replace, but I took the liberty to
clean out some dead/pointless uses and improve formatting in places due
the shorter expression length.

Instances of "".equals(string) were only changed when it was immediately
obvious that string is not null, e.g., due a preceding null check.
In Checks.java, the parameter change from CharSequence to String means 
that "cs" needs to be renamed.


-Alan.

Re: RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-12 Thread Daniel Fuchs 

Hi Claes,

It might read even better if things like:

+resultString = !specarg.isEmpty() ? specarg.intern(): opt;

were changed into:

+resultString = specarg.isEmpty() ? opt : specarg.intern();

best regards,

-- daniel

On 12/12/2018 16:53, Claes Redestad wrote:

Hi,

please review this patch to use String.isEmpty when applicable:

Webrev: http://cr.openjdk.java.net/~redestad/8215281/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215281

Why?
- It reads better :-)
- Better startup/warmup due fewer method invocations
- Better peak performance: String.isEmpty is ~1.2x faster than
   String.length

Most changes are simple search and replace, but I took the liberty to
clean out some dead/pointless uses and improve formatting in places due
the shorter expression length.

Instances of "".equals(string) were only changed when it was immediately
obvious that string is not null, e.g., due a preceding null check.

Thanks!

/Claes

RFR: 8215281: Use String.isEmpty() where applicable in java.base

2018-12-12 Thread Claes Redestad 

Hi,

please review this patch to use String.isEmpty when applicable:

Webrev: http://cr.openjdk.java.net/~redestad/8215281/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215281

Why?
- It reads better :-)
- Better startup/warmup due fewer method invocations
- Better peak performance: String.isEmpty is ~1.2x faster than
  String.length

Most changes are simple search and replace, but I took the liberty to
clean out some dead/pointless uses and improve formatting in places due
the shorter expression length.

Instances of "".equals(string) were only changed when it was immediately
obvious that string is not null, e.g., due a preceding null check.

Thanks!

/Claes