OFBiz › OFBiz - Dev

Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

Scott Gray-2

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, [hidden email] 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")
>
>

Adrian Crum-3

Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java

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 <[hidden email]>:

> 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, [hidden email] 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")
>>
>>
>
>

Scott Gray-2

Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java

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, [hidden email] 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 <[hidden email]>:
>
>> 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, [hidden email] 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")
>>>
>>>
>>
>>
>
>
>

Adrian Crum-3

Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java

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, [hidden email] 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 <[hidden email]>:
>>
>>> 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, [hidden email] 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")
>>>>
>>>>
>>>
>>
>>

Scott Gray-2

Re: svn commit: r1372738 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java

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, [hidden email] 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 <[hidden email]>:
>>>
>>>> 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, [hidden email] 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")
>>>>>
>>>>>
>>>>
>>>
>>>
>