On Jun 26, 2014, at 11:58 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:

Otavio,

I skimmed over the patches and they look ok to me. I think they would be suitable for inclusion in jdk9/dev.

Seems fine to me (pending a full test run). Just a syntax niggle in the following, too many brackets: +++ new/src/share/classes/javax/management/modelmbean/RequiredModelMBean.java 2014-06-14 10:16:02.486298421 -0300 @@ -544,7 +544,7 @@ } // convert seconds to milliseconds for time comparison - currencyPeriod = ((new Long(expTime)).longValue()) * 1000; + currencyPeriod = ((Long.valueOf(expTime)).longValue()) * 1000; if (currencyPeriod < 0) { /* if currencyTimeLimit is -1 then value is never cached */ returnCachedValue = false; @@ -580,7 +580,7 @@ if (tStamp == null) tStamp = "0"; - long lastTime = (new Long(tStamp)).longValue(); + long lastTime = (Long.valueOf(tStamp)).longValue(); Paul.

On Jun 26, 2014, at 3:29 PM, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:

Hi Paul,

Seems fine to me (pending a full test run).

Just a syntax niggle in the following, too many brackets:

+++ new/src/share/classes/javax/management/modelmbean/RequiredModelMBean.java 2014-06-14 10:16:02.486298421 -0300 @@ -544,7 +544,7 @@ }

// convert seconds to milliseconds for time comparison - currencyPeriod = ((new Long(expTime)).longValue()) * 1000; + currencyPeriod = ((Long.valueOf(expTime)).longValue()) * 1000; if (currencyPeriod < 0) { /* if currencyTimeLimit is -1 then value is never cached */ returnCachedValue = false; @@ -580,7 +580,7 @@ if (tStamp == null) tStamp = "0";

- long lastTime = (new Long(tStamp)).longValue(); + long lastTime = (Long.valueOf(tStamp)).longValue();

Paul.

Wouldn't it be better to use here Long.parseLong(String)? Long.valueOf(String) would always create a new object here, as expTime and tStamp represents the time and they are never in the range [-128;127].

Well spotted, there is no need to box at all. There are a couple of other cases too (and probably more, need to look more carefully): --- old/src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java 2014-06-14 10:15:58.646298533 -0300 +++ new/src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java 2014-06-14 10:15:58.514298537 -0300 @@ -87,7 +87,7 @@ * represented as a long. */ public UnixNumericUserPrincipal(long name) { - this.name = (new Long(name)).toString(); + this.name = Long.valueOf(name).toString(); <--- this.name = Long.toString(name); } /** @@ -113,7 +113,7 @@ * <code>UnixNumericUserPrincipal</code> as a long. */ public long longValue() { - return ((new Long(name)).longValue()); + return Long.valueOf(name); <--- Long.parseLong(name); } Paul.

Hi Pavel, the both web revs looks identical to me. Here is what I have found so far in the webrev.01: in src/share/classes/com/sun/security/auth/SolarisNumericGroupPrincipal.java: @@ -108,11 +108,11 @@ * @param primaryGroup true if the specified GID represents the * primary group to which this user belongs. * */ public SolarisNumericGroupPrincipal(long name, boolean primaryGroup) { - this.name = (new Long(name)).toString(); + this.name = Long.valueOf(name).toString(); this.primaryGroup = primaryGroup; } It is better to use Long.toString(long): + this.name = Long.toString(name); This also applies to: src/share/classes/com/sun/security/auth/SolarisNumericUserPrincipal.java src/share/classes/com/sun/security/auth/UnixNumericGroupPrincipal.java src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java @@ -94,11 +94,11 @@ * * @param name the user identification number (UID) for this user * represented as a long. */ public SolarisNumericUserPrincipal(long name) { - this.name = (new Long(name)).toString(); + this.name = Long.valueOf(name).toString(); } In src/share/classes/javax/management/modelmbean/RequiredModelMBean.java: @@ -542,11 +542,11 @@ RequiredModelMBean.class.getName(), mth,"currencyTimeLimit: " + expTime); } // convert seconds to milliseconds for time comparison - currencyPeriod = ((new Long(expTime)).longValue()) * 1000; + currencyPeriod = ((Long.valueOf(expTime)).longValue()) * 1000; Please use Long.parseLong(String), e.g.: + currencyPeriod = Long.parseLong(expTime) * 1000; And here please use Long.parseLong8String) too: @@ -578,11 +578,11 @@ } if (tStamp == null) tStamp = "0"; - long lastTime = (new Long(tStamp)).longValue(); + long lastTime = (Long.valueOf(tStamp)).longValue(); e.g.: + long lastTime = Long.parseLong(tStamp); In src/share/classes/sun/security/jgss/wrapper/GSSNameElement.java @@ -201,11 +201,11 @@ return false; } } public int hashCode() { - return new Long(pName).hashCode(); + return Long.valueOf(pName).hashCode(); } The method Long.hashCode(long) (added in JDK 8) should be used to calculate the hash for a long value, e.g.: + return Long.hashCode(pName); Best regards, Andrej Golovnin On Fri, Jun 27, 2014 at 12:00 PM, Pavel Rappo <pavel.rappo@oracle.com> wrote:

I created an issue to track the progress and also made 2 webrevs. One for the original patch and one for the changes that have been suggested earlier in this thread by Paul and Andrej. Here we go:

https://bugs.openjdk.java.net/browse/JDK-8048267

http://cr.openjdk.java.net/~prappo/8048267/webrev.00 http://cr.openjdk.java.net/~prappo/8048267/webrev.01

-Pavel

On 26 Jun 2014, at 10:58, Chris Hegarty <chris.hegarty@oracle.com> wrote:

...

Otavio,

I skimmed over the patches and they look ok to me. I think they would be suitable for inclusion in jdk9/dev.

-Chris.

P.S. I do not currently have time to sponsor this, but I cc’ed Pavel who may be able to help get his in.

On 14 Jun 2014, at 14:46, Otávio Gonçalves de Santana < otaviojava@java.net> wrote:

...

Reason: The Long class has cache and using it, will save memory and will faster than using create new instance of Long.

webrev: https://dl.dropboxusercontent.com/u/16109193/open_jdk/long_value_of.zip

similar: https://bugs.openjdk.java.net/browse/JDK-8044461 -- Otávio Gonçalves de Santana

blog: http://otaviosantana.blogspot.com.br/ twitter: http://twitter.com/otaviojava site: http://www.otaviojava.com.br (11) 98255-3513

<sun_tools.diff><sun_security.diff><sun_nio.diff><sun_management.diff><sun_jvmstat.diff><javax_swing.diff><javax-management.diff><java_text.diff><java_awt_image.diff><internal_org_objectweb.diff><com_sun_tools.diff><com_sun_security.diff><com_sun_jmx_snmp.diff><com_sun_jdni_ldap.diff><com_sun_imageio.diff>

I've just created a webrev with all the changes we've discussed so far. Plus some more I've spotted while looking into the code. Please note, this webrev is not incremental. It grabs all the changes between the original patch and the latest discussed: http://cr.openjdk.java.net/~prappo/8048267/webrev.02 Andrej, you can verify that all your changes are included as well as Paul's. As a workaround for this particular review thread you can download changeset files from consecutive webrevs and just diff them: http://cr.openjdk.java.net/~prappo/8048267/webrev.$(i)/jdk.changeset http://cr.openjdk.java.net/~prappo/8048267/webrev.$(i+1)/jdk.changeset In addition to these I also found some more. It turns out such simple changes can lead to infinite recursion calls. Compare these: (line 622): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/com/s... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/com/s... (lines 384, 508): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/java/... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/java/... Also, I removed one useless creation of a Long object here: (line 191): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/s... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/s... I wonder if we should leave a cast to int here: (line 383): http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/m... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/m... Well it's probably nothing to worry about, but strictly speaking this changes the behaviour. Before the change, long was truncated to fit int. And now it's not. P.S. Andrej, it looks like you don't have an 'Author' status. Well, that's a pity. We could mention you in the 'Reviewed-by' line. Your contributions are really good. -Pavel On 27 Jun 2014, at 11:52, Pavel Rappo <pavel.rappo@oracle.com> wrote:

Hi Andrej,

They are not identical. Maybe it's just hard to spot it as it's not an incremental webrev. Have a look at

(line 583):

http://cr.openjdk.java.net/~prappo/8048267/webrev.00/src/share/classes/javax... http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/javax...

(line 90):

http://cr.openjdk.java.net/~prappo/8048267/webrev.00/src/share/classes/com/s... http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/com/s...

Though I've missed one you suggested in RequiredModelMBean.java:547 Anyway, I'll try to incorporate everything you've spotted so far.

Thanks, -Pavel

On 27 Jun 2014, at 11:36, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:

...

Hi Pavel,

the both web revs looks identical to me. Here is what I have found so far in the webrev.01:

in src/share/classes/com/sun/security/auth/SolarisNumericGroupPrincipal.java:

@@ -108,11 +108,11 @@ * @param primaryGroup true if the specified GID represents the * primary group to which this user belongs. * */ public SolarisNumericGroupPrincipal(long name, boolean primaryGroup) { - this.name = (new Long(name)).toString(); + this.name = Long.valueOf(name).toString(); this.primaryGroup = primaryGroup; }

It is better to use Long.toString(long):

+ this.name = Long.toString(name);

This also applies to:

src/share/classes/com/sun/security/auth/SolarisNumericUserPrincipal.java src/share/classes/com/sun/security/auth/UnixNumericGroupPrincipal.java src/share/classes/com/sun/security/auth/UnixNumericUserPrincipal.java

@@ -94,11 +94,11 @@ * * @param name the user identification number (UID) for this user * represented as a long. */ public SolarisNumericUserPrincipal(long name) { - this.name = (new Long(name)).toString(); + this.name = Long.valueOf(name).toString(); }

In src/share/classes/javax/management/modelmbean/RequiredModelMBean.java:

@@ -542,11 +542,11 @@ RequiredModelMBean.class.getName(), mth,"currencyTimeLimit: " + expTime); }

// convert seconds to milliseconds for time comparison - currencyPeriod = ((new Long(expTime)).longValue()) * 1000; + currencyPeriod = ((Long.valueOf(expTime)).longValue()) * 1000;

Please use Long.parseLong(String), e.g.:

+ currencyPeriod = Long.parseLong(expTime) * 1000;

And here please use Long.parseLong8String) too:

@@ -578,11 +578,11 @@ }

if (tStamp == null) tStamp = "0";

- long lastTime = (new Long(tStamp)).longValue(); + long lastTime = (Long.valueOf(tStamp)).longValue();

e.g.:

+ long lastTime = Long.parseLong(tStamp);

In src/share/classes/sun/security/jgss/wrapper/GSSNameElement.java

@@ -201,11 +201,11 @@ return false; } }

public int hashCode() { - return new Long(pName).hashCode(); + return Long.valueOf(pName).hashCode(); }

The method Long.hashCode(long) (added in JDK 8) should be used to calculate the hash for a long value, e.g.:

+ return Long.hashCode(pName);

Best regards, Andrej Golovnin

On Fri, Jun 27, 2014 at 12:00 PM, Pavel Rappo <pavel.rappo@oracle.com> wrote: I created an issue to track the progress and also made 2 webrevs. One for the original patch and one for the changes that have been suggested earlier in this thread by Paul and Andrej. Here we go:

https://bugs.openjdk.java.net/browse/JDK-8048267

http://cr.openjdk.java.net/~prappo/8048267/webrev.00 http://cr.openjdk.java.net/~prappo/8048267/webrev.01

-Pavel

On 26 Jun 2014, at 10:58, Chris Hegarty <chris.hegarty@oracle.com> wrote:

...

Otavio,

I skimmed over the patches and they look ok to me. I think they would be suitable for inclusion in jdk9/dev.

-Chris.

P.S. I do not currently have time to sponsor this, but I cc’ed Pavel who may be able to help get his in.

On 14 Jun 2014, at 14:46, Otávio Gonçalves de Santana <otaviojava@java.net> wrote:

...

Reason: The Long class has cache and using it, will save memory and will faster than using create new instance of Long.

webrev: https://dl.dropboxusercontent.com/u/16109193/open_jdk/long_value_of.zip

similar: https://bugs.openjdk.java.net/browse/JDK-8044461 -- Otávio Gonçalves de Santana

blog: http://otaviosantana.blogspot.com.br/ twitter: http://twitter.com/otaviojava site: http://www.otaviojava.com.br (11) 98255-3513 <sun_tools.diff><sun_security.diff><sun_nio.diff><sun_management.diff><sun_jvmstat.diff><javax_swing.diff><javax-management.diff><java_text.diff><java_awt_image.diff><internal_org_objectweb.diff><com_sun_tools.diff><com_sun_security.diff><com_sun_jmx_snmp.diff><com_sun_jdni_ldap.diff><com_sun_imageio.diff>

I found more two unnecessary valueOf. About Andrej, is it not possible add two people in "Contributed-by:" tag? diff -r d02b062bc827 src/share/classes/com/sun/tools/example/debug/tty/Commands.java --- a/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 13 11:21:30 2014 -0700 +++ b/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 27 20:06:28 2014 -0300 @@ -935,7 +935,7 @@ try { methodInfo = loc.sourceName() + MessageOutput.format("line number", - new Object [] {new Long(lineNumber)}); + new Object [] {lineNumber}); } catch (AbsentInformationException e) { methodInfo = MessageOutput.format("unknown"); } @@ -946,7 +946,7 @@ meth.declaringType().name(), meth.name(), methodInfo, - new Long(pc)}); + pc}); } else { MessageOutput.println("stack frame dump", new Object [] {new Integer(frameNumber + 1), On Fri, Jun 27, 2014 at 5:16 PM, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:

Hi Pavel,

I'm not sure what the style guide for the source code says, but there is a space between the cast operator and the field name in src/share/classes/com/sun/jmx/snmp/daemon/SnmpAdaptorServer.java (line 881):

@Override public Long getSnmpOutGenErrs() { - return new Long(snmpOutGenErrs); + return (long) snmpOutGenErrs;

}

In all other changes there is no space after the cast operator.

...

Also, I removed one useless creation of a Long object here:

(line 191):

http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/s...

...

http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/s...

And I have found one more :-) in KerberosTime.java:

252 public int getSeconds() { 253 Long temp_long = kerberosTime / 1000L; 254 return temp_long.intValue(); 255 }

This can be changed to:

252 public int getSeconds() { 253 long temp_long = kerberosTime / 1000L; 254 return (int) temp_long; 255 }

...

I wonder if we should leave a cast to int here:

(line 383):

http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/m...

...

http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/m...

...

Well it's probably nothing to worry about, but strictly speaking this

changes the behaviour. Before the change, long was truncated to fit int. And now it's not.

I would not change the behavior now. I think it is better to file a new issue and change it in a separate commit. Having this change in a separate commit may simplify tracking this change back in case it would cause some problems (I don't believe it). And in the new issue you may provide better description for this change.

And a minor comment for JvmMemoryImpl.java: Maybe it is better to use Long0 in the line 387. So the code of the method JvmMemoryImpl.getJvmMemoryPendingFinalCount() will look similar to the code of other methods in the JvmMemoryImpl class. They all use the Long0 constant to return 0L.

In sun/management/snmp/jvminstr/JvmThreadingImpl.java in the line 313:

312 public Long getJvmThreadPeakCount() throws SnmpStatusException { 313 return (long)getThreadMXBean().getPeakThreadCount(); 314 }

There is one space too much between "return" and the cast operator. The additional space was in the original version too, but maybe we can clean up here the code a little bit.

...

P.S. Andrej, it looks like you don't have an 'Author' status. Well,

that's a pity. We could mention you in the 'Reviewed-by' line. Your contributions are really good.

Thanks! But I don't really care about it as long as I can help to improve the overall code quality.

Best regards, Andrej Golovnin

-- Atenciosamente. Otávio Gonçalves de Santana blog: http://otaviosantana.blogspot.com.br/ twitter: http://twitter.com/otaviojava site: http://www.otaviojava.com.br (11) 98255-3513

I've updated the webrev. It includes all the changes we've discussed so far plus these: http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/macosx/classes/sun/... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/com/s... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/r... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/r... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/r... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/share/classes/sun/r... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/solaris/classes/jav... http://cr.openjdk.java.net/~prappo/8048267/webrev.03/src/solaris/classes/sun... Andrej, about the 0L -> Long0 change. I think it's not necessary since this case is *maybe* different from others. There's even a comment on it: // Should never happen... but stay safe all the same. // else return 0L; Anyway at a runtime 0L and Long0 will be the same object. So don't worry about that. I think we're almost ready to go. Thanks, -Pavel On 28 Jun 2014, at 00:09, Otávio Gonçalves de Santana <otaviojava@java.net> wrote:

I found more two unnecessary valueOf. About Andrej, is it not possible add two people in "Contributed-by:" tag?

diff -r d02b062bc827 src/share/classes/com/sun/tools/example/debug/tty/Commands.java --- a/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 13 11:21:30 2014 -0700 +++ b/src/share/classes/com/sun/tools/example/debug/tty/Commands.java Fri Jun 27 20:06:28 2014 -0300 @@ -935,7 +935,7 @@ try { methodInfo = loc.sourceName() + MessageOutput.format("line number", - new Object [] {new Long(lineNumber)}); + new Object [] {lineNumber}); } catch (AbsentInformationException e) { methodInfo = MessageOutput.format("unknown"); } @@ -946,7 +946,7 @@ meth.declaringType().name(), meth.name(), methodInfo, - new Long(pc)}); + pc}); } else { MessageOutput.println("stack frame dump", new Object [] {new Integer(frameNumber + 1),

On Fri, Jun 27, 2014 at 5:16 PM, Andrej Golovnin <andrej.golovnin@gmail.com> wrote: Hi Pavel,

I'm not sure what the style guide for the source code says, but there is a space between the cast operator and the field name in src/share/classes/com/sun/jmx/snmp/daemon/SnmpAdaptorServer.java (line 881):

@Override public Long getSnmpOutGenErrs() { - return new Long(snmpOutGenErrs); + return (long) snmpOutGenErrs;

}

In all other changes there is no space after the cast operator.

...

Also, I removed one useless creation of a Long object here:

(line 191):

http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/s... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/s...

And I have found one more :-) in KerberosTime.java:

252 public int getSeconds() { 253 Long temp_long = kerberosTime / 1000L; 254 return temp_long.intValue(); 255 }

This can be changed to:

252 public int getSeconds() { 253 long temp_long = kerberosTime / 1000L; 254 return (int) temp_long; 255 }

...

I wonder if we should leave a cast to int here:

(line 383):

http://cr.openjdk.java.net/~prappo/8048267/webrev.01/src/share/classes/sun/m... http://cr.openjdk.java.net/~prappo/8048267/webrev.02/src/share/classes/sun/m...

Well it's probably nothing to worry about, but strictly speaking this changes the behaviour. Before the change, long was truncated to fit int. And now it's not.

I would not change the behavior now. I think it is better to file a new issue and change it in a separate commit. Having this change in a separate commit may simplify tracking this change back in case it would cause some problems (I don't believe it). And in the new issue you may provide better description for this change.

And a minor comment for JvmMemoryImpl.java: Maybe it is better to use Long0 in the line 387. So the code of the method JvmMemoryImpl.getJvmMemoryPendingFinalCount() will look similar to the code of other methods in the JvmMemoryImpl class. They all use the Long0 constant to return 0L.

In sun/management/snmp/jvminstr/JvmThreadingImpl.java in the line 313:

312 public Long getJvmThreadPeakCount() throws SnmpStatusException { 313 return (long)getThreadMXBean().getPeakThreadCount(); 314 }

There is one space too much between "return" and the cast operator. The additional space was in the original version too, but maybe we can clean up here the code a little bit.

...

P.S. Andrej, it looks like you don't have an 'Author' status. Well, that's a pity. We could mention you in the 'Reviewed-by' line. Your contributions are really good.

Thanks! But I don't really care about it as long as I can help to improve the overall code quality.

Best regards, Andrej Golovnin

-- Atenciosamente.

Otávio Gonçalves de Santana

blog: http://otaviosantana.blogspot.com.br/ twitter: http://twitter.com/otaviojava site: http://www.otaviojava.com.br (11) 98255-3513