You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Anthony Whitford (JIRA)" <ji...@apache.org> on 2009/12/22 10:12:29 UTC
[jira] Created: (LANG-575) HashCodeBuilder reflectionAppend creates
unnecessary copy of excludeFields
HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
--------------------------------------------------------------------------
Key: LANG-575
URL: https://issues.apache.org/jira/browse/LANG-575
Project: Commons Lang
Issue Type: Bug
Components: lang.builder.*
Affects Versions: 2.4
Environment: Sun Java JDK 1.6.0_17
Reporter: Anthony Whitford
See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
{code}
List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
{code}
Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (LANG-575) HashCodeBuilder reflectionAppend
creates unnecessary copy of excludeFields
Posted by "Anthony Whitford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/LANG-575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12793561#action_12793561 ]
Anthony Whitford commented on LANG-575:
---------------------------------------
Considering how repetitive this issue is, you may want to consider LANG-283 because there does seem to be a refactoring opportunity here.
> HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
> --------------------------------------------------------------------------
>
> Key: LANG-575
> URL: https://issues.apache.org/jira/browse/LANG-575
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.builder.*
> Affects Versions: 2.4
> Environment: Sun Java JDK 1.6.0_17
> Reporter: Anthony Whitford
>
> See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
> Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
> {code}
> List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
> {code}
> Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
> I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
> I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (LANG-575) HashCodeBuilder reflectionAppend creates
unnecessary copy of excludeFields
Posted by "Niall Pemberton (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/LANG-575?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Niall Pemberton updated LANG-575:
---------------------------------
Fix Version/s: (was: 3.0)
2.5
> HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
> --------------------------------------------------------------------------
>
> Key: LANG-575
> URL: https://issues.apache.org/jira/browse/LANG-575
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.builder.*
> Affects Versions: 2.4
> Environment: Sun Java JDK 1.6.0_17
> Reporter: Anthony Whitford
> Fix For: 2.5
>
>
> See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
> Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
> {code}
> List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
> {code}
> Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
> I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
> I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (LANG-575) HashCodeBuilder reflectionAppend creates
unnecessary copy of excludeFields
Posted by "Henri Yandell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/LANG-575?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Henri Yandell closed LANG-575.
------------------------------
Resolution: Fixed
svn ci -m "Replacing the creation of a List in the core of each Builder class to test contains on the excludeFields with a call to ArrayUtils.contains. Reported by Anthony Whitford in LANG-575"
Sending src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java
Sending src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java
Sending src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java
Transmitting file data ...
Committed revision 897421.
> HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
> --------------------------------------------------------------------------
>
> Key: LANG-575
> URL: https://issues.apache.org/jira/browse/LANG-575
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.builder.*
> Affects Versions: 2.4
> Environment: Sun Java JDK 1.6.0_17
> Reporter: Anthony Whitford
> Fix For: 3.0
>
>
> See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
> Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
> {code}
> List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
> {code}
> Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
> I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
> I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (LANG-575) HashCodeBuilder reflectionAppend
creates unnecessary copy of excludeFields
Posted by "Anthony Whitford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/LANG-575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12793560#action_12793560 ]
Anthony Whitford commented on LANG-575:
---------------------------------------
Note that *EqualsBuilder* has the exact same issue.
See: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/EqualsBuilder.java?view=markup
*reflectionAppend*, line 321:
{code}
List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
{code}
And so does *CompareToBuilder*...
See: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/CompareToBuilder.java?view=markup
*reflectionAppend*, line 356:
{code}
List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
{code}
> HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
> --------------------------------------------------------------------------
>
> Key: LANG-575
> URL: https://issues.apache.org/jira/browse/LANG-575
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.builder.*
> Affects Versions: 2.4
> Environment: Sun Java JDK 1.6.0_17
> Reporter: Anthony Whitford
>
> See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
> Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
> {code}
> List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
> {code}
> Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
> I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
> I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (LANG-575) HashCodeBuilder reflectionAppend creates
unnecessary copy of excludeFields
Posted by "Henri Yandell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/LANG-575?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Henri Yandell updated LANG-575:
-------------------------------
Fix Version/s: 3.0
Definitely hoping to challenge the builder code before 3.0 is released. Its got the most duplication and the most JIRA items open.
Thanks for both of your optimization suggestions.
> HashCodeBuilder reflectionAppend creates unnecessary copy of excludeFields
> --------------------------------------------------------------------------
>
> Key: LANG-575
> URL: https://issues.apache.org/jira/browse/LANG-575
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.builder.*
> Affects Versions: 2.4
> Environment: Sun Java JDK 1.6.0_17
> Reporter: Anthony Whitford
> Fix For: 3.0
>
>
> See http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/java/org/apache/commons/lang3/builder/HashCodeBuilder.java?view=markup
> Please review the implementation for *reflectionAppend* (lines 174 to 202)... Specifically, see line 182:
> {code}
> List<String> excludedFieldList = excludeFields != null ? Arrays.asList(excludeFields) : Collections.<String>emptyList();
> {code}
> Note that if you are in the habit of passing in a String array for excluding fields ({{String[] excludeFields}}) -- which is a best practice when using Hibernate (to skip primary keys ({{@id}}) and version fields ({{@version}}) that change upon persistence) -- _EVERY TIME_ the _hashCode_ is calculated, an _ArrayList_ is being created -- generating fodder for the garbage collector.
> I thought I might get around this by passing a {{Collection<String>}} instead of a {{String[]}}, but ironically the implementation of the {{reflectionHashCode(Object object, Collection<String> excludeFields)}} (see lines 475 to 477), for example, transforms the {{Collection<String>}} into a {{String[]}} only to have it transformed internally into a temporary {{ArrayList<String>}}.
> I would expect the implementation to use and read what is submitted, whether that is a {{String[]}} or a {{Collection<String>}}. I don't think it needs to create another copy just to have a convenient {{contains}} method. Efficiency is important, especially in the event of rehashing.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.