[mapguide-internals] Code Reveiw request: Remove IsNull check when get property from feature source

Thu Aug 11 11:12:17 EDT 2011

Hi Buddy,

I do not have specific percentage metrics on null versus not null.  A "select *" from a database is more likely to produce nulls since it brings back all columns in a table instead of specific columns.  For clean data (no nulls), your patch will improve performance.  As a best practise we may want to guide our user base to only select the FDO properties they need.

Perhaps someone else on the list has specific examples/metrics on occurrences of null property values.  Also, early test builds of MapGuide 2.4 could be used by MapGuide users to see if there are any performance implications.  The change in behaviour should be documented in the release notes so our user base knows what to look for.

And a code review of the stack unwinds for the "Get" methods would help ensure that no memory leaks occur.  This was also be easy to verify with sample data containing nulls.   

Regards,
Trevor

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Buddy Hu
Sent: August 11, 2011 7:25 AM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] Code Reveiw request: Remove IsNull check when get property from feature source

Hi Trevor,

I agree with you and Traian that we should push down the IsNull to FDO providers.
For this patch, we replace the IsNull check with try-catch, doesn't make thing worse. As Traian said, it makes Server faster for most data sets. Although for data sets with lot of null values, it makes MG slower.
Are you sure the Null values are common? If the Null values are more than non-Null values, I think this patch doesn't help to improve the MG performance but hurt.

Regards,
Buddy

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Trevor Wekel
Sent: Wednesday, August 10, 2011 10:40 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] Code Reveiw request: Remove IsNull check when get property from feature source

I agree with Traian.  Faster IsNull processing should be pushed down to FDO.  I assume there is a ReadNext() call made to Fdo before these methods are called.  Perhaps the entire record including "null" state for each property could be cached during the ReadNext() call?  Nulls  are fairly common, especially in databases, so this exception will get hit regularly.

The other issue is memory leaks in FDO and MapGuide.  Since nulls are common, the stack unwind should be checked for all providers and all "Get" calls to ensure that no memory leaks will occur.

Regards,
Trevor

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Martin Morrison
Sent: August 10, 2011 5:57 AM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] Code Reveiw request: Remove IsNull check when get property from feature source

The only gotcha is NULL is a valid value.  MapGuide/FDO should handle it better.

Martin Morrison
Application Engineer
Engineering Design Systems, Inc.
3780 Peters Creek Rd Ext SW
Roanoke, VA  24018
540.345.1410
gis.edsi.com

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Zac Spitzer
Sent: Wednesday, August 10, 2011 4:12 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Code Reveiw request: Remove IsNull check when get property from feature source

as most, if not all have databases have a not null flag per column?

that metadata could help with handling nullable columns?

it does get tricky with the different handling of empty strings

On Wed, Aug 10, 2011 at 4:43 PM, Traian Stanev <traian.stanev at autodesk.com> wrote:
>
> The initial implementation for the rendering code indeed used optimistic null handling via try-catch instead of checking for IsNull, for performance reasons. For most data sets, not checking for null is a lot faster, because in FDO checking for IsNull gets the property value a second time. However, there were some sample data sets with lots of null values, which incurred a big overhead because of the number of catches that happened -- so someone added the IsNull. For such data, having the IsNull was a lot faster due to the inherent slowness of catching.
>
> The facts have not changed since then -- removing the null check will make things faster for data sets with no null values but slower for data sets with lots of null values.
>
> Really, MapGuide should not have to work around FDO being slow. There must be some way to optimize FDO providers where this is slow to cache the value after checking it for null and then quickly return it knowing that the next property value requested will likely be for the property that was previously checked for null.
>
> Traian
>
>
>
>
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org
> [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Zac 
> Spitzer
> Sent: Wednesday, August 10, 2011 12:36 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] Code Reveiw request: Remove IsNull 
> check when get property from feature source
>
> that old thread was titled [mapguide-internals] Exception thrown on null values?
>
> here is some of it, dates back to 08, which was pre-nabble days?
>
> http://osgeo-org.1803224.n2.nabble.com/RE-mapguide-internals-Exception
> -thrown-on-null-values-td2051195.html
>
> from my *limited* understanding, this patch catches any exceptions and 
> then gracefully handles them which should be a big win
>
> z
>
>
> On Wed, Aug 10, 2011 at 2:28 PM, Jason Birch <jason at jasonbirch.com> wrote:
>> Wasn't the isnull check initially put in place to deal with stability 
>> issues (something to do with FDO exceptions not unwinding properly or
>> something?)
>>  It was a long time ago, but it sounds pretty familiar.
>>
>> On 1 August 2011 01:15, Buddy Hu wrote:
>>
>>> Hi Lists,
>>>
>>> Please add your comments for this patch:
>>> http://trac.osgeo.org/mapguide/ticket/1766
>>>
>>> Regards,
>>> Buddy
>>> _______________________________________________
>>> mapguide-internals mailing list
>>> mapguide-internals at lists.osgeo.org
>>> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>>>
>> _______________________________________________
>> mapguide-internals mailing list
>> mapguide-internals at lists.osgeo.org
>> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>>
>
>
>
> --
> Zac Spitzer
> Solution Architect / Director
> Ennoble Consultancy Australia
> http://www.ennoble.com.au
> http://zacster.blogspot.com
> +61 405 847 168
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>

--
Zac Spitzer
Solution Architect / Director
Ennoble Consultancy Australia
http://www.ennoble.com.au
http://zacster.blogspot.com
+61 405 847 168
_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals