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 <[email protected]
<mailto:[email protected]>> 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, [email protected]
<mailto:[email protected]>
Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09 <tel:%2B33%204%2076%2018%2080%2009> Fax
: +33 4 76 18 80 23 <tel:%2B33%204%2076%2018%2080%2023>
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/
<http://cr.openjdk.java.net/%7Efparain/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"
<[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>> wrote:
This a simple fix to solve CR 6988220:
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=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/
<http://cr.openjdk.java.net/%7E__fparain/6988220/webrev.00/>
<http://cr.openjdk.java.net/~fparain/6988220/webrev.00/
<http://cr.openjdk.java.net/%7Efparain/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
<tel:%2B33%204%2076%2018%2081%2017>
<tel:%2B33%204%2076%2018%2081%2017>
Email: [email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
