You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Scott Gray <sc...@hotwaxmedia.com> on 2012/08/14 08:11:45 UTC
Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
Hi Adrian,
Are you sure those are optimizations? For getBigDecimal and getDouble I think almost every call would cause the exception to be thrown, is that actually more efficient than an instanceof check? I know both are expensive but I don't know which is more so.
Thanks
Scott
On 14/08/2012, at 6:02 PM, adrianc@apache.org wrote:
> Author: adrianc
> Date: Tue Aug 14 06:02:58 2012
> New Revision: 1372738
>
> URL: http://svn.apache.org/viewvc?rev=1372738&view=rev
> Log:
> Small GenericEntity optimization - remove instanceof checks in getXxx methods.
>
> Modified:
> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>
> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1372738&r1=1372737&r2=1372738&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Tue Aug 14 06:02:58 2012
> @@ -619,14 +619,8 @@ public class GenericEntity extends Obser
> }
>
> public String getString(String name) {
> - // might be nice to add some ClassCastException handling... and auto conversion? hmmm...
> Object object = get(name);
> - if (object == null) return null;
> - if (object instanceof java.lang.String) {
> - return (String) object;
> - } else {
> - return object.toString();
> - }
> + return object == null ? null : object.toString();
> }
>
> public java.sql.Timestamp getTimestamp(String name) {
> @@ -656,22 +650,28 @@ public class GenericEntity extends Obser
> public Double getDouble(String name) {
> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
> Object value = get(name);
> - if (value instanceof BigDecimal) {
> - return Double.valueOf(((BigDecimal) value).doubleValue());
> - } else {
> - return (Double) value;
> + if (value != null) {
> + try {
> + BigDecimal numValue = (BigDecimal) value;
> + return new Double(numValue.doubleValue());
> + } catch (ClassCastException e) {
> + }
> }
> + return (Double) value;
> }
>
> public BigDecimal getBigDecimal(String name) {
> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
> // NOTE: for things to generally work properly BigDecimal should really be used as the java-type in the field type def XML files
> Object value = get(name);
> - if (value instanceof Double) {
> - return BigDecimal.valueOf(((Double) value).doubleValue());
> - } else {
> - return (BigDecimal) value;
> + if (value != null) {
> + try {
> + Double numValue = (Double) value;
> + return new BigDecimal(numValue.doubleValue());
> + } catch (ClassCastException e) {
> + }
> }
> + return (BigDecimal) value;
> }
>
> @SuppressWarnings("deprecation")
>
>
Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
Thanks Adrian. I wonder when we'd be safe to remove those checks altogether.
Regards
Scott
On 15/08/2012, at 6:25 PM, Adrian Crum wrote:
> Understood. I will revert the two methods.
>
> -Adrian
>
> On 8/14/2012 11:48 PM, Scott Gray wrote:
>> Taking getBigDecimal for example, where 99.999% of the time the object will actually be a BigDecimal, we're now throwing a ClassCastException followed by a cast. I'm not sure how that translates to one object type test?
>>
>> The Double -> BigDecimal and vice-versa for getDouble are old corner cases left over from the conversion to BigDecimal a few years back.
>>
>> Based on a quick google, the speed difference between throwing an exception vs. instanceof seems negligible. Additionally it is often argued that relying on exceptions instead of performing proper checks is considered bad practice (hence them being called exceptions).
>>
>> I don't really mind either way but this just seems like a style preference thing rather than an actual optimization.
>>
>> Regards
>> Scott
>>
>> On 14/08/2012, at 9:11 PM, adrian.crum@sandglass-software.com wrote:
>>
>>> The original code had an instanceof and a cast. So, in the case where instanceof evaluates to true, there will be two object type tests - instanceof and the cast. The modification guarantees there will be only one object type test.
>>>
>>> I've seen this optimization used elsewhere, but I haven't actually measured it.
>>>
>>> -Adrian
>>>
>>> Quoting Scott Gray <sc...@hotwaxmedia.com>:
>>>
>>>> Hi Adrian,
>>>>
>>>> Are you sure those are optimizations? For getBigDecimal and getDouble I think almost every call would cause the exception to be thrown, is that actually more efficient than an instanceof check? I know both are expensive but I don't know which is more so.
>>>>
>>>> Thanks
>>>> Scott
>>>>
>>>> On 14/08/2012, at 6:02 PM, adrianc@apache.org wrote:
>>>>
>>>>> Author: adrianc
>>>>> Date: Tue Aug 14 06:02:58 2012
>>>>> New Revision: 1372738
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1372738&view=rev
>>>>> Log:
>>>>> Small GenericEntity optimization - remove instanceof checks in getXxx methods.
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>>
>>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1372738&r1=1372737&r2=1372738&view=diff
>>>>> ==============================================================================
>>>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original)
>>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Tue Aug 14 06:02:58 2012
>>>>> @@ -619,14 +619,8 @@ public class GenericEntity extends Obser
>>>>> }
>>>>>
>>>>> public String getString(String name) {
>>>>> - // might be nice to add some ClassCastException handling... and auto conversion? hmmm...
>>>>> Object object = get(name);
>>>>> - if (object == null) return null;
>>>>> - if (object instanceof java.lang.String) {
>>>>> - return (String) object;
>>>>> - } else {
>>>>> - return object.toString();
>>>>> - }
>>>>> + return object == null ? null : object.toString();
>>>>> }
>>>>>
>>>>> public java.sql.Timestamp getTimestamp(String name) {
>>>>> @@ -656,22 +650,28 @@ public class GenericEntity extends Obser
>>>>> public Double getDouble(String name) {
>>>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>>>> Object value = get(name);
>>>>> - if (value instanceof BigDecimal) {
>>>>> - return Double.valueOf(((BigDecimal) value).doubleValue());
>>>>> - } else {
>>>>> - return (Double) value;
>>>>> + if (value != null) {
>>>>> + try {
>>>>> + BigDecimal numValue = (BigDecimal) value;
>>>>> + return new Double(numValue.doubleValue());
>>>>> + } catch (ClassCastException e) {
>>>>> + }
>>>>> }
>>>>> + return (Double) value;
>>>>> }
>>>>>
>>>>> public BigDecimal getBigDecimal(String name) {
>>>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>>>> // NOTE: for things to generally work properly BigDecimal should really be used as the java-type in the field type def XML files
>>>>> Object value = get(name);
>>>>> - if (value instanceof Double) {
>>>>> - return BigDecimal.valueOf(((Double) value).doubleValue());
>>>>> - } else {
>>>>> - return (BigDecimal) value;
>>>>> + if (value != null) {
>>>>> + try {
>>>>> + Double numValue = (Double) value;
>>>>> + return new BigDecimal(numValue.doubleValue());
>>>>> + } catch (ClassCastException e) {
>>>>> + }
>>>>> }
>>>>> + return (BigDecimal) value;
>>>>> }
>>>>>
>>>>> @SuppressWarnings("deprecation")
>>>>>
>>>>>
>>>>
>>>
>>>
>
Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
Posted by Adrian Crum <ad...@sandglass-software.com>.
Understood. I will revert the two methods.
-Adrian
On 8/14/2012 11:48 PM, Scott Gray wrote:
> Taking getBigDecimal for example, where 99.999% of the time the object will actually be a BigDecimal, we're now throwing a ClassCastException followed by a cast. I'm not sure how that translates to one object type test?
>
> The Double -> BigDecimal and vice-versa for getDouble are old corner cases left over from the conversion to BigDecimal a few years back.
>
> Based on a quick google, the speed difference between throwing an exception vs. instanceof seems negligible. Additionally it is often argued that relying on exceptions instead of performing proper checks is considered bad practice (hence them being called exceptions).
>
> I don't really mind either way but this just seems like a style preference thing rather than an actual optimization.
>
> Regards
> Scott
>
> On 14/08/2012, at 9:11 PM, adrian.crum@sandglass-software.com wrote:
>
>> The original code had an instanceof and a cast. So, in the case where instanceof evaluates to true, there will be two object type tests - instanceof and the cast. The modification guarantees there will be only one object type test.
>>
>> I've seen this optimization used elsewhere, but I haven't actually measured it.
>>
>> -Adrian
>>
>> Quoting Scott Gray <sc...@hotwaxmedia.com>:
>>
>>> Hi Adrian,
>>>
>>> Are you sure those are optimizations? For getBigDecimal and getDouble I think almost every call would cause the exception to be thrown, is that actually more efficient than an instanceof check? I know both are expensive but I don't know which is more so.
>>>
>>> Thanks
>>> Scott
>>>
>>> On 14/08/2012, at 6:02 PM, adrianc@apache.org wrote:
>>>
>>>> Author: adrianc
>>>> Date: Tue Aug 14 06:02:58 2012
>>>> New Revision: 1372738
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1372738&view=rev
>>>> Log:
>>>> Small GenericEntity optimization - remove instanceof checks in getXxx methods.
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>>
>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1372738&r1=1372737&r2=1372738&view=diff
>>>> ==============================================================================
>>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original)
>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Tue Aug 14 06:02:58 2012
>>>> @@ -619,14 +619,8 @@ public class GenericEntity extends Obser
>>>> }
>>>>
>>>> public String getString(String name) {
>>>> - // might be nice to add some ClassCastException handling... and auto conversion? hmmm...
>>>> Object object = get(name);
>>>> - if (object == null) return null;
>>>> - if (object instanceof java.lang.String) {
>>>> - return (String) object;
>>>> - } else {
>>>> - return object.toString();
>>>> - }
>>>> + return object == null ? null : object.toString();
>>>> }
>>>>
>>>> public java.sql.Timestamp getTimestamp(String name) {
>>>> @@ -656,22 +650,28 @@ public class GenericEntity extends Obser
>>>> public Double getDouble(String name) {
>>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>>> Object value = get(name);
>>>> - if (value instanceof BigDecimal) {
>>>> - return Double.valueOf(((BigDecimal) value).doubleValue());
>>>> - } else {
>>>> - return (Double) value;
>>>> + if (value != null) {
>>>> + try {
>>>> + BigDecimal numValue = (BigDecimal) value;
>>>> + return new Double(numValue.doubleValue());
>>>> + } catch (ClassCastException e) {
>>>> + }
>>>> }
>>>> + return (Double) value;
>>>> }
>>>>
>>>> public BigDecimal getBigDecimal(String name) {
>>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>>> // NOTE: for things to generally work properly BigDecimal should really be used as the java-type in the field type def XML files
>>>> Object value = get(name);
>>>> - if (value instanceof Double) {
>>>> - return BigDecimal.valueOf(((Double) value).doubleValue());
>>>> - } else {
>>>> - return (BigDecimal) value;
>>>> + if (value != null) {
>>>> + try {
>>>> + Double numValue = (Double) value;
>>>> + return new BigDecimal(numValue.doubleValue());
>>>> + } catch (ClassCastException e) {
>>>> + }
>>>> }
>>>> + return (BigDecimal) value;
>>>> }
>>>>
>>>> @SuppressWarnings("deprecation")
>>>>
>>>>
>>>
>>
>>
Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
Posted by Scott Gray <sc...@hotwaxmedia.com>.
Taking getBigDecimal for example, where 99.999% of the time the object will actually be a BigDecimal, we're now throwing a ClassCastException followed by a cast. I'm not sure how that translates to one object type test?
The Double -> BigDecimal and vice-versa for getDouble are old corner cases left over from the conversion to BigDecimal a few years back.
Based on a quick google, the speed difference between throwing an exception vs. instanceof seems negligible. Additionally it is often argued that relying on exceptions instead of performing proper checks is considered bad practice (hence them being called exceptions).
I don't really mind either way but this just seems like a style preference thing rather than an actual optimization.
Regards
Scott
On 14/08/2012, at 9:11 PM, adrian.crum@sandglass-software.com wrote:
> The original code had an instanceof and a cast. So, in the case where instanceof evaluates to true, there will be two object type tests - instanceof and the cast. The modification guarantees there will be only one object type test.
>
> I've seen this optimization used elsewhere, but I haven't actually measured it.
>
> -Adrian
>
> Quoting Scott Gray <sc...@hotwaxmedia.com>:
>
>> Hi Adrian,
>>
>> Are you sure those are optimizations? For getBigDecimal and getDouble I think almost every call would cause the exception to be thrown, is that actually more efficient than an instanceof check? I know both are expensive but I don't know which is more so.
>>
>> Thanks
>> Scott
>>
>> On 14/08/2012, at 6:02 PM, adrianc@apache.org wrote:
>>
>>> Author: adrianc
>>> Date: Tue Aug 14 06:02:58 2012
>>> New Revision: 1372738
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1372738&view=rev
>>> Log:
>>> Small GenericEntity optimization - remove instanceof checks in getXxx methods.
>>>
>>> Modified:
>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>>
>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1372738&r1=1372737&r2=1372738&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original)
>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Tue Aug 14 06:02:58 2012
>>> @@ -619,14 +619,8 @@ public class GenericEntity extends Obser
>>> }
>>>
>>> public String getString(String name) {
>>> - // might be nice to add some ClassCastException handling... and auto conversion? hmmm...
>>> Object object = get(name);
>>> - if (object == null) return null;
>>> - if (object instanceof java.lang.String) {
>>> - return (String) object;
>>> - } else {
>>> - return object.toString();
>>> - }
>>> + return object == null ? null : object.toString();
>>> }
>>>
>>> public java.sql.Timestamp getTimestamp(String name) {
>>> @@ -656,22 +650,28 @@ public class GenericEntity extends Obser
>>> public Double getDouble(String name) {
>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>> Object value = get(name);
>>> - if (value instanceof BigDecimal) {
>>> - return Double.valueOf(((BigDecimal) value).doubleValue());
>>> - } else {
>>> - return (Double) value;
>>> + if (value != null) {
>>> + try {
>>> + BigDecimal numValue = (BigDecimal) value;
>>> + return new Double(numValue.doubleValue());
>>> + } catch (ClassCastException e) {
>>> + }
>>> }
>>> + return (Double) value;
>>> }
>>>
>>> public BigDecimal getBigDecimal(String name) {
>>> // this "hack" is needed for now until the Double/BigDecimal issues are all resolved
>>> // NOTE: for things to generally work properly BigDecimal should really be used as the java-type in the field type def XML files
>>> Object value = get(name);
>>> - if (value instanceof Double) {
>>> - return BigDecimal.valueOf(((Double) value).doubleValue());
>>> - } else {
>>> - return (BigDecimal) value;
>>> + if (value != null) {
>>> + try {
>>> + Double numValue = (Double) value;
>>> + return new BigDecimal(numValue.doubleValue());
>>> + } catch (ClassCastException e) {
>>> + }
>>> }
>>> + return (BigDecimal) value;
>>> }
>>>
>>> @SuppressWarnings("deprecation")
>>>
>>>
>>
>>
>
>
>
Re: svn commit: r1372738 -
/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
Posted by ad...@sandglass-software.com.
The original code had an instanceof and a cast. So, in the case where
instanceof evaluates to true, there will be two object type tests -
instanceof and the cast. The modification guarantees there will be
only one object type test.
I've seen this optimization used elsewhere, but I haven't actually
measured it.
-Adrian
Quoting Scott Gray <sc...@hotwaxmedia.com>:
> Hi Adrian,
>
> Are you sure those are optimizations? For getBigDecimal and
> getDouble I think almost every call would cause the exception to be
> thrown, is that actually more efficient than an instanceof check? I
> know both are expensive but I don't know which is more so.
>
> Thanks
> Scott
>
> On 14/08/2012, at 6:02 PM, adrianc@apache.org wrote:
>
>> Author: adrianc
>> Date: Tue Aug 14 06:02:58 2012
>> New Revision: 1372738
>>
>> URL: http://svn.apache.org/viewvc?rev=1372738&view=rev
>> Log:
>> Small GenericEntity optimization - remove instanceof checks in
>> getXxx methods.
>>
>> Modified:
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>>
>> Modified:
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1372738&r1=1372737&r2=1372738&view=diff
>> ==============================================================================
>> ---
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java
>> (original)
>> +++
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Tue
>> Aug 14 06:02:58 2012
>> @@ -619,14 +619,8 @@ public class GenericEntity extends Obser
>> }
>>
>> public String getString(String name) {
>> - // might be nice to add some ClassCastException
>> handling... and auto conversion? hmmm...
>> Object object = get(name);
>> - if (object == null) return null;
>> - if (object instanceof java.lang.String) {
>> - return (String) object;
>> - } else {
>> - return object.toString();
>> - }
>> + return object == null ? null : object.toString();
>> }
>>
>> public java.sql.Timestamp getTimestamp(String name) {
>> @@ -656,22 +650,28 @@ public class GenericEntity extends Obser
>> public Double getDouble(String name) {
>> // this "hack" is needed for now until the
>> Double/BigDecimal issues are all resolved
>> Object value = get(name);
>> - if (value instanceof BigDecimal) {
>> - return Double.valueOf(((BigDecimal) value).doubleValue());
>> - } else {
>> - return (Double) value;
>> + if (value != null) {
>> + try {
>> + BigDecimal numValue = (BigDecimal) value;
>> + return new Double(numValue.doubleValue());
>> + } catch (ClassCastException e) {
>> + }
>> }
>> + return (Double) value;
>> }
>>
>> public BigDecimal getBigDecimal(String name) {
>> // this "hack" is needed for now until the
>> Double/BigDecimal issues are all resolved
>> // NOTE: for things to generally work properly BigDecimal
>> should really be used as the java-type in the field type def XML
>> files
>> Object value = get(name);
>> - if (value instanceof Double) {
>> - return BigDecimal.valueOf(((Double) value).doubleValue());
>> - } else {
>> - return (BigDecimal) value;
>> + if (value != null) {
>> + try {
>> + Double numValue = (Double) value;
>> + return new BigDecimal(numValue.doubleValue());
>> + } catch (ClassCastException e) {
>> + }
>> }
>> + return (BigDecimal) value;
>> }
>>
>> @SuppressWarnings("deprecation")
>>
>>
>
>