Re: Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

[Kurchi Hazra]

Hi,

  Please ignore the previous webrev and see this instead:
http://cr.openjdk.java.net/~khazra/7146763/webrev.03/

This has Stuart's suggestion integrated correctly. In addition, I 
realized that
make/sun/rmi/rmic/Makefile is not yet ready to have the 
JAVAC_WARNINGS_FATAL
flag turned on, since it implicitly also builds files from sun/tools 
with more then 400

warnings in them. The change in this file has now been removed.

- Kurchi



On 2/24/2012 11:01 AM, Kurchi Hazra wrote:

Hi Stuart,

   Thanks for the detailed explanation. Here is an updated webrev:
http://cr.openjdk.java.net/~khazra/7146763/webrev.02/


- Kurchi

On 2/24/2012 12:54 AM, Stuart Marks wrote:

On 2/22/12 1:25 PM, Kurchi Hazra wrote:

On 2/22/2012 10:01 AM, Rémi Forax wrote:

Hi Kurchi, hi all,

in ReliableLog, you can get ride of the @SupressWarnings,
getLogClassConstructor should return a Constructor and not a 
Constructor
extends LogFile>,
the field logClassConstructor should be typed Constructor and
in openLogFile, the log should be constructed like this

log = (logClassConstructor == null ?
new LogFile(logName, "rw") :

(LogFile)logClassConstructor.newInstance(logName, "rw"));


The idea is that a cast on a LogFile is typesafe but not a cast on a
Constructor.


  If I change the return type to Constructor, I get the following 
error:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error:
incompatible types
 logClassConstructor = getLogClassConstructor();
 ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?
And the following warning:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: 
warning:

[unchecked] unchecked cast
 cl.getConstructor(String.class, 
String.class);

  ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?


Thanks,
Kurchi


Hi Kurchi,

To implement Rémi's suggestion fully, you would also have to change 
the type of logClassConstructor to Contructor near line 122, 
remove the cast of cl.getConstructor() near line 350, and then add 
the cast to LogFile at the call to newInstance() near line 546.


This works to get rid of the warnings and errors, but the declaration 
of Constructor is somewhat imprecise. The code checks to make sure 
that the loaded class is a subclass of LogFile (that's what the 
isAssignableFrom check is doing). Thus the type of the loaded class 
really should be Class, and correspondingly the 
logClassConstructor should be Constructor. That's 
how logClassConstructor is declared now and it would be nice to keep 
it that way.


It turns out that Class.asSubclass() does this conversion without 
generating an unchecked warning. This internally does an 
isAssignableFrom() check and casts to the right wildcarded type, so 
this can simplify the code in getLogClassConstructor() somewhat as 
well. (Incidentally, asSubClass() has @SuppressWarnings on its 
implementation.) I've appended some diffs below (to be applied on top 
of your most recent webrev) to show how this can be done.


The behavior is slightly different, as it throws ClassCastException 
(which is caught by the catch clause below, emitting a log message) 
instead of silently returning null. This is probably an improvement, 
since if the user specifies the wrong class in the property name, the 
exception stack trace should indicate what happened.


s'marks




diff -r 72d32fd57d89 src/share/classes/sun/rmi/log/ReliableLog.java
--- a/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 
00:01:53 2012 -0800
+++ b/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 
00:39:02 2012 -0800

@@ -330,9 +330,7 @@
  * property a) can be loaded, b) is a subclass of LogFile, and 
c) has a
  * public two-arg constructor (String, String); otherwise 
returns null.

  **/
-@SuppressWarnings("unchecked")
-private static Constructor
-getLogClassConstructor() {
+private static Constructor 
getLogClassConstructor() {


 String logClassName = AccessController.doPrivileged(
 new GetPropertyAction("sun.rmi.log.class"));
@@ -345,11 +343,9 @@
return 
ClassLoader.getSystemClassLoader();

 }
 });
-Class cl = loader.loadClass(logClassName);
-if (LogFile.class.isAssignableFrom(cl)) {
-return (Constructor)
-cl.getConstructor(String.class, 
String.class);

-}
+Class cl =
+
loader.loadClass(logClassName).asSubclass(LogFile.class);

+return cl.getConstructor(String.class, String.class);
 } catch (Ex

Re: Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

[Rémi Forax]

On 02/24/2012 08:01 PM, Kurchi Hazra wrote:

Hi Stuart,

   Thanks for the detailed explanation. Here is an updated webrev:
http://cr.openjdk.java.net/~khazra/7146763/webrev.02/


Hi Kurchi,
the field logClassConstructor should not be changed after all,
it should be declared as a Constructor so you can remove
the cast to LogFile at line 542.

with this change, the patch looks good to me.




- Kurchi


cheers,
Rémi

hg: jdk8/tl/jdk: 7079093: TEST_BUG: java/lang/instrument/ManifestTest.sh fails with cygwin

[staffan . larsen]

Changeset: 4893a89b4916
Author:sla
Date:  2012-02-24 20:09 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4893a89b4916

7079093: TEST_BUG: java/lang/instrument/ManifestTest.sh fails with cygwin
Summary: Work around problems in some cygwin installations
Reviewed-by: alanb, sspitsyn

! test/ProblemList.txt
! test/java/lang/instrument/ManifestTest.sh

hg: jdk8/tl/jdk: 7073626: RmiBootstrapTest.sh and RmiSslBootstrapTest.sh fail under Cygwin

[staffan . larsen]

Changeset: 585f2c72d042
Author:sla
Date:  2012-02-24 20:02 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/585f2c72d042

7073626: RmiBootstrapTest.sh and RmiSslBootstrapTest.sh fail under Cygwin
Summary: Detect and handle cygwin correctly
Reviewed-by: alanb, sspitsyn

! test/ProblemList.txt
! test/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh

Re: Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

[Kurchi Hazra]

Hi Stuart,

   Thanks for the detailed explanation. Here is an updated webrev:
http://cr.openjdk.java.net/~khazra/7146763/webrev.02/


- Kurchi

On 2/24/2012 12:54 AM, Stuart Marks wrote:

On 2/22/12 1:25 PM, Kurchi Hazra wrote:

On 2/22/2012 10:01 AM, Rémi Forax wrote:

Hi Kurchi, hi all,

in ReliableLog, you can get ride of the @SupressWarnings,
getLogClassConstructor should return a Constructor and not a 
Constructor
extends LogFile>,
the field logClassConstructor should be typed Constructor and
in openLogFile, the log should be constructed like this

log = (logClassConstructor == null ?
new LogFile(logName, "rw") :

(LogFile)logClassConstructor.newInstance(logName, "rw"));


The idea is that a cast on a LogFile is typesafe but not a cast on a
Constructor.


  If I change the return type to Constructor, I get the following 
error:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error:
incompatible types
 logClassConstructor = getLogClassConstructor();
 ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?
And the following warning:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: warning:
[unchecked] unchecked cast
 cl.getConstructor(String.class, 
String.class);

  ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?


Thanks,
Kurchi


Hi Kurchi,

To implement Rémi's suggestion fully, you would also have to change 
the type of logClassConstructor to Contructor near line 122, remove 
the cast of cl.getConstructor() near line 350, and then add the cast 
to LogFile at the call to newInstance() near line 546.


This works to get rid of the warnings and errors, but the declaration 
of Constructor is somewhat imprecise. The code checks to make sure 
that the loaded class is a subclass of LogFile (that's what the 
isAssignableFrom check is doing). Thus the type of the loaded class 
really should be Class, and correspondingly the 
logClassConstructor should be Constructor. That's 
how logClassConstructor is declared now and it would be nice to keep 
it that way.


It turns out that Class.asSubclass() does this conversion without 
generating an unchecked warning. This internally does an 
isAssignableFrom() check and casts to the right wildcarded type, so 
this can simplify the code in getLogClassConstructor() somewhat as 
well. (Incidentally, asSubClass() has @SuppressWarnings on its 
implementation.) I've appended some diffs below (to be applied on top 
of your most recent webrev) to show how this can be done.


The behavior is slightly different, as it throws ClassCastException 
(which is caught by the catch clause below, emitting a log message) 
instead of silently returning null. This is probably an improvement, 
since if the user specifies the wrong class in the property name, the 
exception stack trace should indicate what happened.


s'marks




diff -r 72d32fd57d89 src/share/classes/sun/rmi/log/ReliableLog.java
--- a/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 
00:01:53 2012 -0800
+++ b/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 
00:39:02 2012 -0800

@@ -330,9 +330,7 @@
  * property a) can be loaded, b) is a subclass of LogFile, and c) 
has a
  * public two-arg constructor (String, String); otherwise returns 
null.

  **/
-@SuppressWarnings("unchecked")
-private static Constructor
-getLogClassConstructor() {
+private static Constructor 
getLogClassConstructor() {


 String logClassName = AccessController.doPrivileged(
 new GetPropertyAction("sun.rmi.log.class"));
@@ -345,11 +343,9 @@
return 
ClassLoader.getSystemClassLoader();

 }
 });
-Class cl = loader.loadClass(logClassName);
-if (LogFile.class.isAssignableFrom(cl)) {
-return (Constructor)
-cl.getConstructor(String.class, 
String.class);

-}
+Class cl =
+
loader.loadClass(logClassName).asSubclass(LogFile.class);

+return cl.getConstructor(String.class, String.class);
 } catch (Exception e) {
 System.err.println("Exception occurred:");
 e.printStackTrace();




--
-Kurchi

hg: jdk8/tl/langtools: 7137836: tidy up Names.java

[jonathan . gibbons]

Changeset: e6b5c3aff85c
Author:jjg
Date:  2012-02-24 10:40 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/e6b5c3aff85c

7137836: tidy up Names.java
Reviewed-by: mcimadamore

! src/share/classes/com/sun/tools/javac/util/Names.java

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Eamonn McManus]

Hi Dan,

I got a bounce from serviceability-dev because I wasn't subscribed to
it, but the message went out to core-libs-dev because I was subscribed
to that. That probably explains what you saw.

Regards,
Éamonn


On 24 February 2012 09:33, Daniel D. Daugherty
 wrote:
>
> Just FYI: I haven't seen Éamonn's posting come in. Just replies to
> his posting. This may mean that other comments are stuck in the
> ether somewhere...
>
> I suspect that the OpenJDK list server is again having issues...
>
> Dan
>
>
> On 2/24/12 8:21 AM, Olivier Lagneau wrote:
>
> I think I have not been clear enough here.
>
> I Agree with Eammon's argument, and anyway ok with this change.
>
> Olivier.
>
>
> Olivier Lagneau said  on date 2/24/2012 12:38 PM:
>
> Hi Éamonn,
>
> Eamonn McManus said  on date 2/23/2012 8:44 PM:
>
> I am not sure it is worth the complexity of extra checks. Do you have data 
> showing that ObjectName.equals usually returns false?In a successful HashMap 
> lookup, for example, it will usually return true since the equals method is 
> used to guard against collisions, and collisions are rare by design. 
> Meanwhile, String.equals is intrinsic in HotSpot so we may assume that it is 
> highly optimized, and you are giving up that optimization if you use other 
> comparisons.
>
> Don't have this kind of data indeed. I don't know of any benchmark/data about 
> usage of ObjectName.equals()
> in most applications. That would be needed to evaluate the exact impact of 
> the change.
> And I agree with the argument that usual semantics of an equals call is to 
> check for equality,
> not the difference.
>
> My argument is mainly that we are moving from comparing identity to equality.
> Thus there will be an impact on the throughput of equals, possibly impacting
> some applications.
>
> Olivier.
>
> Éamonn
>
>
> On 23 February 2012 10:52, Olivier Lagneau  > wrote:
>
>     Hi Frederic,
>
>     Performance and typo comments.
>
>     Regarding performance of ObjectName.equals method, which is certainely
>     a frequent call on ObjectNames, I think that using inner fields
>     (Property array for canonical name and domain length) would be
>     more efficient
>     than using String.equals() on these potentially very long strings.
>
>     Two differents objectNames may often have the same length with
>     different key/properties values, and may often be different only
>     on the last property of the canonical name.
>
>     The Property array field ca_array (comparing length and property
>     contents)
>     and domain length are good candidates to filter out more efficiently
>     different objectNames, knowing that String.equals will compare every
>     single char of the two char arrays.
>
>     So for performance purpose, I suggest to filter out different
>     objectNames
>     by doing inner comparisons in the following order : length of the two
>     canonical names, then domain_length, then ca_array size, then its
>     content,
>     and lastly if all of this fails to filter out, then use String.equals.
>
>  _canonicalName = (new String(canonical_chars, 0, prop_index));
>
>     Typo : useless parentheses.
>
>     Thanks,
>     Olivier.
>
>     -- Olivier Lagneau, olivier.lagn...@oracle.com
>     
>     Oracle, Grenoble Engineering Center - France
>     Phone : +33 4 76 18 80 09  Fax
>     : +33 4 76 18 80 23 
>
>
>
>
>     Frederic Parain said  on date 2/23/2012 6:01 PM:
>
>     No particular reason. But after thinking more about it,
>     equals() should be a better choice, clearer code, and
>     the length check in equals() implementation is likely
>     to help performance of ObjectName's comparisons as
>     ObjectNames are often long with a common section at the
>     beginning.
>
>     I've updated the webrev:
>     http://cr.openjdk.java.net/~fparain/6988220/webrev.01/
>     
>
>     Thanks,
>
>     Fred
>
>     On 2/23/12 4:58 PM, Vitaly Davidovich wrote:
>
>     Hi Frederic,
>
>     Just curious - why are you checking string equality via
>     compareTo()
>     instead of equals()?
>
>     Thanks
>
>     Sent from my phone
>
>     On Feb 23, 2012 10:37 AM, "Frederic Parain"
>          
>          >> wrote:
>
>    This a simple fix to solve CR 6988220:
>     http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220
>     
>
>    The use of String.intern() in the ObjectName class prevents
>    the class the scale well when more than 20K ObjectNames are
>    managed. The fix simply removes the use of Strin

RE: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Iris Clark]

Hi, Dan.

> Just FYI: I haven't seen Éamonn's posting come in. Just replies to his
> posting. This may mean that other comments are stuck in the ether 
> somewhere...
> 
> I suspect that the OpenJDK list server is again having issues...

I just checked the core-libs-dev admin interface to see if anything was waiting 
for moderation, but there wasn't anything.  (Not that I'd expect that to be the 
case for messages from Éamonn.)  

Must be a failure at some other level...  Sorry.

iris

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Daniel D. Daugherty]

Just FYI: I haven't seen Éamonn's posting come in. Just replies to
his posting. This may mean that other comments are stuck in the
ether somewhere...

I suspect that the OpenJDK list server is again having issues...

Dan


On 2/24/12 8:21 AM, Olivier Lagneau wrote:

I think I have not been clear enough here.

I Agree with Eammon's argument, and anyway ok with this change.

Olivier.


Olivier Lagneau said  on date 2/24/2012 12:38 PM:

Hi Éamonn,

Eamonn McManus said  on date 2/23/2012 8:44 PM:
I am not sure it is worth the complexity of extra checks. Do you 
have data showing that ObjectName.equals usually returns false?In a 
successful HashMap lookup, for example, it will usually return true 
since the equals method is used to guard against collisions, and 
collisions are rare by design. Meanwhile, String.equals is intrinsic 
in HotSpot so we may assume that it is highly optimized, and you are 
giving up that optimization if you use other comparisons. 
Don't have this kind of data indeed. I don't know of any 
benchmark/data about usage of ObjectName.equals()
in most applications. That would be needed to evaluate the exact 
impact of the change.
And I agree with the argument that usual semantics of an equals call 
is to check for equality,

not the difference.

My argument is mainly that we are moving from comparing identity to 
equality.
Thus there will be an impact on the throughput of equals, possibly 
impacting

some applications.

Olivier.


Éamonn


On 23 February 2012 10:52, Olivier Lagneau 
mailto:olivier.lagn...@oracle.com>> wrote:


Hi Frederic,

Performance and typo comments.

Regarding performance of ObjectName.equals method, which is 
certainely

a frequent call on ObjectNames, I think that using inner fields
(Property array for canonical name and domain length) would be
more efficient
than using String.equals() on these potentially very long strings.

Two differents objectNames may often have the same length with
different key/properties values, and may often be different only
on the last property of the canonical name.

The Property array field ca_array (comparing length and property
contents)
and domain length are good candidates to filter out more 
efficiently
different objectNames, knowing that String.equals will compare 
every

single char of the two char arrays.

So for performance purpose, I suggest to filter out different
objectNames
by doing inner comparisons in the following order : length of 
the two

canonical names, then domain_length, then ca_array size, then its
content,
and lastly if all of this fails to filter out, then use 
String.equals.


 _canonicalName = (new String(canonical_chars, 0, prop_index));

Typo : useless parentheses.

Thanks,
Olivier.

-- Olivier Lagneau, olivier.lagn...@oracle.com

Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09  Fax
: +33 4 76 18 80 23 




Frederic Parain said  on date 2/23/2012 6:01 PM:

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.01/


Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via
compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, "Frederic Parain"
mailto:frederic.par...@oracle.com>
>> wrote:

   This a simple fix to solve CR 6988220:
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220


   The use of String.intern() in the ObjectName class 
prevents
   the class the scale well when more than 20K 
ObjectNames are
   managed. The fix simply removes the use of 
String.intern(),
   and uses regular String instead. The Object.equals() 
method

   is modified too to make a regular String comparison. The
   complexity of this method now depends on the length of
   the ObjectName's canonical name, and is not impacted any
   more by the number of ObjectName instances being 
handled.


   The Webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.00/

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Olivier Lagneau]

I think I have not been clear enough here.

I Agree with Eammon's argument, and anyway ok with this change.

Olivier.


Olivier Lagneau said  on date 2/24/2012 12:38 PM:

Hi Éamonn,

Eamonn McManus said  on date 2/23/2012 8:44 PM:
I am not sure it is worth the complexity of extra checks. Do you have 
data showing that ObjectName.equals usually returns false?In a 
successful HashMap lookup, for example, it will usually return true 
since the equals method is used to guard against collisions, and 
collisions are rare by design. Meanwhile, String.equals is intrinsic 
in HotSpot so we may assume that it is highly optimized, and you are 
giving up that optimization if you use other comparisons. 
Don't have this kind of data indeed. I don't know of any 
benchmark/data about usage of ObjectName.equals()
in most applications. That would be needed to evaluate the exact 
impact of the change.
And I agree with the argument that usual semantics of an equals call 
is to check for equality,

not the difference.

My argument is mainly that we are moving from comparing identity to 
equality.
Thus there will be an impact on the throughput of equals, possibly 
impacting

some applications.

Olivier.


Éamonn


On 23 February 2012 10:52, Olivier Lagneau 
mailto:olivier.lagn...@oracle.com>> wrote:


Hi Frederic,

Performance and typo comments.

Regarding performance of ObjectName.equals method, which is 
certainely

a frequent call on ObjectNames, I think that using inner fields
(Property array for canonical name and domain length) would be
more efficient
than using String.equals() on these potentially very long strings.

Two differents objectNames may often have the same length with
different key/properties values, and may often be different only
on the last property of the canonical name.

The Property array field ca_array (comparing length and property
contents)
and domain length are good candidates to filter out more efficiently
different objectNames, knowing that String.equals will compare every
single char of the two char arrays.

So for performance purpose, I suggest to filter out different
objectNames
by doing inner comparisons in the following order : length of the 
two

canonical names, then domain_length, then ca_array size, then its
content,
and lastly if all of this fails to filter out, then use 
String.equals.


 _canonicalName = (new String(canonical_chars, 0, prop_index));

Typo : useless parentheses.

Thanks,
Olivier.

-- Olivier Lagneau, olivier.lagn...@oracle.com

Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09  Fax
: +33 4 76 18 80 23 




Frederic Parain said  on date 2/23/2012 6:01 PM:

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.01/


Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via
compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, "Frederic Parain"
mailto:frederic.par...@oracle.com>
>> wrote:

   This a simple fix to solve CR 6988220:

http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220



   The use of String.intern() in the ObjectName class 
prevents
   the class the scale well when more than 20K 
ObjectNames are
   managed. The fix simply removes the use of 
String.intern(),
   and uses regular String instead. The Object.equals() 
method

   is modified too to make a regular String comparison. The
   complexity of this method now depends on the length of
   the ObjectName's canonical name, and is not impacted any
   more by the number of ObjectName instances being handled.

   The Webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.00/

>

   I've tested this fix with the jdk_lang and jdk_management
   test suites.

   Thanks,

   Fred

   --
   

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Olivier Lagneau]

Hi Éamonn,

Eamonn McManus said  on date 2/23/2012 8:44 PM:
I am not sure it is worth the complexity of extra checks. Do you have 
data showing that ObjectName.equals usually returns false?In a 
successful HashMap lookup, for example, it will usually return true 
since the equals method is used to guard against collisions, and 
collisions are rare by design. Meanwhile, String.equals is intrinsic 
in HotSpot so we may assume that it is highly optimized, and you are 
giving up that optimization if you use other comparisons. 
Don't have this kind of data indeed. I don't know of any benchmark/data 
about usage of ObjectName.equals()
in most applications. That would be needed to evaluate the exact impact 
of the change.
And I agree with the argument that usual semantics of an equals call is 
to check for equality,

not the difference.

My argument is mainly that we are moving from comparing identity to 
equality.

Thus there will be an impact on the throughput of equals, possibly impacting
some applications.

Olivier.


Éamonn


On 23 February 2012 10:52, Olivier Lagneau > wrote:


Hi Frederic,

Performance and typo comments.

Regarding performance of ObjectName.equals method, which is certainely
a frequent call on ObjectNames, I think that using inner fields
(Property array for canonical name and domain length) would be
more efficient
than using String.equals() on these potentially very long strings.

Two differents objectNames may often have the same length with
different key/properties values, and may often be different only
on the last property of the canonical name.

The Property array field ca_array (comparing length and property
contents)
and domain length are good candidates to filter out more efficiently
different objectNames, knowing that String.equals will compare every
single char of the two char arrays.

So for performance purpose, I suggest to filter out different
objectNames
by doing inner comparisons in the following order : length of the two
canonical names, then domain_length, then ca_array size, then its
content,
and lastly if all of this fails to filter out, then use String.equals.

 _canonicalName = (new String(canonical_chars, 0, prop_index));

Typo : useless parentheses.

Thanks,
Olivier.

-- Olivier Lagneau, olivier.lagn...@oracle.com

Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09  Fax
: +33 4 76 18 80 23 




Frederic Parain said  on date 2/23/2012 6:01 PM:

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.01/


Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via
compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, "Frederic Parain"
mailto:frederic.par...@oracle.com>
>> wrote:

   This a simple fix to solve CR 6988220:
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220


   The use of String.intern() in the ObjectName class prevents
   the class the scale well when more than 20K ObjectNames are
   managed. The fix simply removes the use of String.intern(),
   and uses regular String instead. The Object.equals() method
   is modified too to make a regular String comparison. The
   complexity of this method now depends on the length of
   the ObjectName's canonical name, and is not impacted any
   more by the number of ObjectName instances being handled.

   The Webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.00/

>

   I've tested this fix with the jdk_lang and jdk_management
   test suites.

   Thanks,

   Fred

   --
   Frederic Parain - Oracle
   Grenoble Engineering Center - France

hg: jdk8/tl/jdk: 7144488: Infinite recursion for some equals tests in Collections

[sean . coffey]

Changeset: 0a350fb8b174
Author:coffeys
Date:  2012-02-24 09:17 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0a350fb8b174

7144488: Infinite recursion for some equals tests in Collections
Reviewed-by: alanb, dholmes, mduigou

! src/share/classes/java/util/Collections.java
+ test/java/util/Collections/EqualsTest.java

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

On 2/23/12 7:46 PM, David Schlosnagle wrote:

Was the main bottleneck the contention on the interned string pool
that prevented concurrent addition of ObjectNames? Are there other
places within the JDK where use of intern() should be analyzed for
similar scalability bottlenecks? I'm also curious what the heap
implications are of no longer using interned strings.


I haven't looked for similar use of intern() within the JDK.
However, the scalability issue of String.intern() is known for
a long term, but the fix is not that simple, as Keith explained,
this is why it has been delayed for a long time due to other
higher priority tasks.


A minor nit is that the equals method could be simplified slightly,
making it more clear that the canonical names must match for equality:

@Override
public boolean equals(Object object)  {
 // same object case
 if (this == object) return true;

 // object is not an object name case
 if (!(object instanceof ObjectName)) return false;
 ObjectName on = (ObjectName) object;
 return _canonicalName.equals(on._canonicalName);
}


Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

hg: jdk8/tl/jdk: 7133138: Improve io performance around timezone lookups

[sean . coffey]

Changeset: a589a8dbde79
Author:coffeys
Date:  2012-02-24 09:10 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a589a8dbde79

7133138: Improve io performance around timezone lookups
Reviewed-by: okutsu

! make/tools/src/build/tools/javazic/Mappings.java
! src/share/classes/sun/util/calendar/ZoneInfo.java
! src/share/classes/sun/util/calendar/ZoneInfoFile.java

RE: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Dmytro Sheyko]

Hi,

Though "compareTo" tends to be less efficient than "equals", it offers better 
type safety.

When we (mistakenly!) compare objects of different type, "equals" silently 
accepts parameter of wrong type,
but returns false. Comparison with "compareTo" is rejected by compiler.

Consider,

String string = ...
Date date = ...

if (string.equals(date)) { // always false
}

if (string.compareTo(date) == 0) { // compilation error
}

Regards,
Dmytro

> Date: Thu, 23 Feb 2012 10:58:47 -0500
> Subject: Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes 
> major performance issues at scale
> From: vita...@gmail.com
> To: frederic.par...@oracle.com
> CC: serviceability-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> 
> Hi Frederic,
> 
> Just curious - why are you checking string equality via compareTo() instead
> of equals()?
> 
> Thanks
> 
> Sent from my phone
> On Feb 23, 2012 10:37 AM, "Frederic Parain" 
> wrote:
> 
> > This a simple fix to solve CR 6988220:
> > http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=6988220
> >
> > The use of String.intern() in the ObjectName class prevents
> > the class the scale well when more than 20K ObjectNames are
> > managed. The fix simply removes the use of String.intern(),
> > and uses regular String instead. The Object.equals() method
> > is modified too to make a regular String comparison. The
> > complexity of this method now depends on the length of
> > the ObjectName's canonical name, and is not impacted any
> > more by the number of ObjectName instances being handled.
> >
> > The Webrev:
> > http://cr.openjdk.java.net/~**fparain/6988220/webrev.00/
> >
> > I've tested this fix with the jdk_lang and jdk_management
> > test suites.
> >
> > Thanks,
> >
> > Fred
> >
> > --
> > Frederic Parain - Oracle
> > Grenoble Engineering Center - France
> > Phone: +33 4 76 18 81 17
> > Email: frederic.par...@oracle.com
> >
> >
  

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

Making String.intern() more scalable doesn't seem to be a
solution for short (or medium?) time frame. Even if the
computation cost of ObjectName.equals() is increased by
this fix, there's no performance measurement in favor or
against this change. I've looked for benchmarks stressing
the ObjectName class, but I haven't found one yet.

Using java.util "Set" or "Map" introduces new issues, like
the memory footprint and new synchronizations to protect
consistency of the collection.

The Ops-Center team is stuck with this scalability issue
for two years now. They have identified the problem in JDK6
and we're not able to provide them with a solution for JDK7
or JDK8.

David, I agree that I have no data about the impact on other
use-cases, but I know that the use of String.intern() cannot
be easily workaround. We can remove the use of String.intern()
and if the performance of the new ObjectName.equals() method
really becomes a performance bottleneck for other use-cases,
them we can re-work this method to improve its performance.
But I'd prefer not starting complex optimizations on a method
without having real indication that it causes real performance
issues.

Fred

On 2/23/12 11:23 PM, Keith McGuigan wrote:


Making String.intern() more scalable has been on our list of
things-to-do for a long, long time. But, it's not trivial. Simply
increasing the size of the hashtable is no good because we'd be upping
our footprint unconditionally, so really we want a growing hashtable
which is a bit more effort (though not impossible, of course, it just
hasn't bubbled up to the top of the priority list).

Another problem with using 'intern()' is that when you intern a string
you're placing it into the permgen, and space there is at a premium. (no
perm gen project will hopefully fix this soon).

If you really want to use == instead of "equals()", you can use a
java.util "set" or "map" data structure and stash all of your strings in
there. Then you'll have canonicalized references that == will work upon,
and you won't run into the intern() scalability (or concurrency) issues.

--
- Keith


On 2/23/2012 4:53 PM, David Holmes wrote:

Hi Fred,

java.lang.ObjectName? :) Need to fix that.

So often we intern strings precisely so that equals() can use == to
improve performance.

It seems to me that this is a case of "fixing" something for one
use-case without knowing what the impact will be on other use-cases!

Is there perhaps a solution that makes String.intern more scalable?

David
-

On 24/02/2012 1:36 AM, Frederic Parain wrote:

This a simple fix to solve CR 6988220:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/

I've tested this fix with the jdk_lang and jdk_management
test suites.

Thanks,

Fred



--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7133138 Improve io performance around timezone lookups

[Masayoshi Okutsu]

The fix looks good.

Thanks,
Masayoshi

On 2/22/2012 3:19 AM, Seán Coffey wrote:
I've worked with Masayoshi on this issue. Hoping to push to JDK8 and 
backport to 7u and a jdk6 once baked for a while.


Some windows boxes were showing performance issues when attempting to 
iterate through all available timezones available in the JRE. Changes 
made here should reduce the amount of file stat calls made on 
jre/lib/zi/* files.  Tweaks are made to the alias table also to reduce 
number of aliases that require lookup.


All TZ regression tests run without issue. I've seen a 20-30% speed up 
on iteration of timezones on windows as a result. Unix doesn't appear 
to suffer as badly from such a file IO hit.


http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7133138
http://cr.openjdk.java.net/~coffeys/webrev.7133138/

regards,
Sean.

Re: Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

[Rémi Forax]

On 02/24/2012 09:54 AM, Stuart Marks wrote:

On 2/22/12 1:25 PM, Kurchi Hazra wrote:

On 2/22/2012 10:01 AM, Rémi Forax wrote:

Hi Kurchi, hi all,

in ReliableLog, you can get ride of the @SupressWarnings,
getLogClassConstructor should return a Constructor and not a 
Constructor
extends LogFile>,
the field logClassConstructor should be typed Constructor and
in openLogFile, the log should be constructed like this

log = (logClassConstructor == null ?
new LogFile(logName, "rw") :

(LogFile)logClassConstructor.newInstance(logName, "rw"));


The idea is that a cast on a LogFile is typesafe but not a cast on a
Constructor.


  If I change the return type to Constructor, I get the following 
error:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error:
incompatible types
 logClassConstructor = getLogClassConstructor();
 ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?
And the following warning:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: warning:
[unchecked] unchecked cast
 cl.getConstructor(String.class, 
String.class);

  ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?


Thanks,
Kurchi


Hi Kurchi,

To implement Rémi's suggestion fully, you would also have to change 
the type of logClassConstructor to Contructor near line 122, remove 
the cast of cl.getConstructor() near line 350, and then add the cast 
to LogFile at the call to newInstance() near line 546.


This works to get rid of the warnings and errors, but the declaration 
of Constructor is somewhat imprecise. The code checks to make sure 
that the loaded class is a subclass of LogFile (that's what the 
isAssignableFrom check is doing). Thus the type of the loaded class 
really should be Class, and correspondingly the 
logClassConstructor should be Constructor. That's 
how logClassConstructor is declared now and it would be nice to keep 
it that way.


It turns out that Class.asSubclass() does this conversion without 
generating an unchecked warning. This internally does an 
isAssignableFrom() check and casts to the right wildcarded type, so 
this can simplify the code in getLogClassConstructor() somewhat as 
well. (Incidentally, asSubClass() has @SuppressWarnings on its 
implementation.) I've appended some diffs below (to be applied on top 
of your most recent webrev) to show how this can be done.


The behavior is slightly different, as it throws ClassCastException 
(which is caught by the catch clause below, emitting a log message) 
instead of silently returning null. This is probably an improvement, 
since if the user specifies the wrong class in the property name, the 
exception stack trace should indicate what happened.


s'marks


Hi Stuart, hi Kurchi,
sorry to not have answer before,
and yes, using asSubClass is better that what I've proposed.

cheers,
Rémi

Re: Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

[Stuart Marks]

On 2/22/12 1:25 PM, Kurchi Hazra wrote:

On 2/22/2012 10:01 AM, Rémi Forax wrote:

Hi Kurchi, hi all,

in ReliableLog, you can get ride of the @SupressWarnings,
getLogClassConstructor should return a Constructor and not a Constructor,
the field logClassConstructor should be typed Constructor and
in openLogFile, the log should be constructed like this

log = (logClassConstructor == null ?
new LogFile(logName, "rw") :
(LogFile)logClassConstructor.newInstance(logName, "rw"));

The idea is that a cast on a LogFile is typesafe but not a cast on a
Constructor.


  If I change the return type to Constructor, I get the following error:
../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error:
incompatible types
 logClassConstructor = getLogClassConstructor();
 ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?
And the following warning:

../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: warning:
[unchecked] unchecked cast
 cl.getConstructor(String.class, String.class);
  ^
   required: Constructor
   found:Constructor
   where CAP#1 is a fresh type-variable:
 CAP#1 extends Object from capture of ?


Thanks,
Kurchi


Hi Kurchi,

To implement Rémi's suggestion fully, you would also have to change the type of 
logClassConstructor to Contructor near line 122, remove the cast of 
cl.getConstructor() near line 350, and then add the cast to LogFile at the call 
to newInstance() near line 546.


This works to get rid of the warnings and errors, but the declaration of 
Constructor is somewhat imprecise. The code checks to make sure that the 
loaded class is a subclass of LogFile (that's what the isAssignableFrom check 
is doing). Thus the type of the loaded class really should be ClassLogFile>, and correspondingly the logClassConstructor should be Constructorextends LogFile>. That's how logClassConstructor is declared now and it would 
be nice to keep it that way.


It turns out that Class.asSubclass() does this conversion without generating an 
unchecked warning. This internally does an isAssignableFrom() check and casts 
to the right wildcarded type, so this can simplify the code in 
getLogClassConstructor() somewhat as well. (Incidentally, asSubClass() has 
@SuppressWarnings on its implementation.) I've appended some diffs below (to be 
applied on top of your most recent webrev) to show how this can be done.


The behavior is slightly different, as it throws ClassCastException (which is 
caught by the catch clause below, emitting a log message) instead of silently 
returning null. This is probably an improvement, since if the user specifies 
the wrong class in the property name, the exception stack trace should indicate 
what happened.


s'marks




diff -r 72d32fd57d89 src/share/classes/sun/rmi/log/ReliableLog.java
--- a/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 00:01:53 
2012 -0800
+++ b/src/share/classes/sun/rmi/log/ReliableLog.javaFri Feb 24 00:39:02 
2012 -0800
@@ -330,9 +330,7 @@
  * property a) can be loaded, b) is a subclass of LogFile, and c) has a
  * public two-arg constructor (String, String); otherwise returns null.
  **/
-@SuppressWarnings("unchecked")
-private static Constructor
-getLogClassConstructor() {
+private static Constructor getLogClassConstructor() {

 String logClassName = AccessController.doPrivileged(
 new GetPropertyAction("sun.rmi.log.class"));
@@ -345,11 +343,9 @@
return ClassLoader.getSystemClassLoader();
 }
 });
-Class cl = loader.loadClass(logClassName);
-if (LogFile.class.isAssignableFrom(cl)) {
-return (Constructor)
-cl.getConstructor(String.class, String.class);
-}
+Class cl =
+loader.loadClass(logClassName).asSubclass(LogFile.class);
+return cl.getConstructor(String.class, String.class);
 } catch (Exception e) {
 System.err.println("Exception occurred:");
 e.printStackTrace();

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

I'm in favor of not adding complexity to ObjectName.equals().
The goal of this CR is to remove a bottleneck created by the use
of String.intern() in ObjectName's constructors. This CR doesn't
aim to optimize the ObjectName.equals() method.

An application can define an optimized method to compare two
ObjectNames based on its knowledge of the patterns used by its
ObjectNames.

However, there's no simple way to workaround the bottleneck created
by the String.intern() call. The Ops-Center team has experimented
the removal of String.intern() by providing their own implementation
of ObjectName and playing with the bootclasspath, but this is not
a solution easy to deploy in a production environment.

Fred

On 2/23/12 8:44 PM, Eamonn McManus wrote:

I am not sure it is worth the complexity of extra checks. Do you have
data showing that ObjectName.equals usually returns false? In a
successful HashMap lookup, for example, it will usually return true
since the equals method is used to guard against collisions, and
collisions are rare by design. Meanwhile, String.equals is intrinsic in
HotSpot so we may assume that it is highly optimized, and you are giving
up that optimization if you use other comparisons.

Éamonn


On 23 February 2012 10:52, Olivier Lagneau mailto:olivier.lagn...@oracle.com>> wrote:

Hi Frederic,

Performance and typo comments.

Regarding performance of ObjectName.equals method, which is certainely
a frequent call on ObjectNames, I think that using inner fields
(Property array for canonical name and domain length) would be more
efficient
than using String.equals() on these potentially very long strings.

Two differents objectNames may often have the same length with
different key/properties values, and may often be different only
on the last property of the canonical name.

The Property array field ca_array (comparing length and property
contents)
and domain length are good candidates to filter out more efficiently
different objectNames, knowing that String.equals will compare every
single char of the two char arrays.

So for performance purpose, I suggest to filter out different
objectNames
by doing inner comparisons in the following order : length of the two
canonical names, then domain_length, then ca_array size, then its
content,
and lastly if all of this fails to filter out, then use String.equals.

  _canonicalName = (new String(canonical_chars, 0, prop_index));

Typo : useless parentheses.

Thanks,
Olivier.

-- Olivier Lagneau, olivier.lagn...@oracle.com

Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09  Fax :
+33 4 76 18 80 23 




Frederic Parain said  on date 2/23/2012 6:01 PM:

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.01/


Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via
compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, "Frederic Parain"
mailto:frederic.par...@oracle.com>
>> wrote:

This a simple fix to solve CR 6988220:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

>

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/