[mapguide-internals] Some code fixes for RFC 60

[Traian Stanev]

Hi UV,

I was specifically referring to the RSCOLORLIST in the rendering service. It's not a list of strings. It's a list of parsed colors. Specifically this section of code:

       945         PRSCOLORLIST tileColorPalette = new RSCOLORLIST(); 
 	946	        /// the map object has a list from all color entries found in the most recent Layerstylization 
 	947	    /// TODO currently they are interpreted as ffffffff 32 bit RGBA string values. 
 	948	    /// adding expresssions and other interpretations could be done here.  
 	949	        try { 
 	950	        ParseColorStrings (tileColorPalette, map); 
 	951	                #ifdef _DEBUG_PNG8 
 	952	                printf("<<<<<<<<<<<<<<<<<<<<< MgServerRenderingService::ColorPalette->size(): %d\n", mapColorPalette->size()); 
 	953	                #endif 
 	954	                //  call the image renderer to create the image ---------------------------------------- 
 	955	                if (wcscmp(m_rendererName.c_str(), L"AGG") == 0) 
 	956	                        data.reset(((AGGRenderer*)dr)->Save(format, saveWidth, saveHeight, tileColorPalette)); 
 	957	                else 
 	958	                        data.reset(((GDRenderer*)dr)->Save(format, saveWidth, saveHeight)); 
 	959	        } catch (exception e) { 
 	960	        if (tileColorPalette)    // cleanup the bare pointer (its a STL list so we do it by hand 
 	961	        { 
 	962	            RSCOLORLIST::iterator it; 
 	963	            for (it = tileColorPalette->begin();it != tileColorPalette->end(); it++)  
 	964	                delete *it; // the RS_Color is on the heap and needs destruction 
 	965	            delete tileColorPalette; 
 	966	        } 
 	967	        ACE_DEBUG((LM_DEBUG, L"(%t) %w caught in RenderingService ColorPaletteGeneration\n", e.what())); 
 	968	                throw e; 
 	969	        }



Thanks,
Traian


-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
Sent: Saturday, May 09, 2009 9:25 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Some code fixes for RFC 60

Hi Traian,

thanks for the comments.

*TYPEDEF
The allcaps typedef followed a coding style I was used to and saw in the 
Oem code base to differentiate the TYPEDEFS from the real type names. 
Sorry for this.  I did not see typedefs in the Mapguide code so I missed 
that bit.

std::list<const MdfString>
You are right that the stringlist is not an efficient way of storing 
this generally.

*Why Strings?
The reason for using the strings is that if the values in the XML are 
not RRGGBBAA hex the color collecting code would still work.
Only the rendering service needs to reinterpret the stringlist.
This is implementing separation of concerns between the collaborating 
parts of the code. Since requests for extending this code have been 
stated already this seemes like the way to simplify the implementation 
and being able to use a simple adapter design to reinterpret the XML 
strings found in the map.

*Why List?
I was originally considering the vector as opposed to the list container 
but the beauty of the sort and unique methods made me use the list.
My reasoning is that I can use the sort/unique functionality of the 
std::list at the cost of less efficient storage of some small data 
structures.

*Extra Processing*
My assessment of the extra cost for the additional processing time once 
per tile was that it was not relevant in comparison to the complexity of 
the mapping algorithms. So if we are looking at an roughly estimated 
ratio of 1000: 1 computations the  cleanness of the code would justify 
this approach.
If this assessment is wrong then this needs changing..... however, the 
costly sort / unique algorithms implemented with a vector might chew up 
the savings made by choosing the vector as a container.

Thanks for pointing those bits out to me as this reasoning should have 
been written in the code where its used or at least in the RFC.
My fault.


Traian Stanev wrote:
> I am looking at the RSCOLORLIST structure in the RenderingService part of the RFC (v8 of the patch file) and I have some comments.
>
> Using pointers allocated with new to point to something as small as a color is not "more flexible". It is:
>
> * A lot slower. Probably at least 10x slower, due to all the calls to new().
> * Uses a lot more memory. A color can be represented in 4 bytes, using RS_Color::argb(). You are using 4 bytes for the pointer, 16 bytes for the color, plus the malloc() overhead per color, which is at least 8 more bytes. 
> * A lot more complicated and leak-prone, in that it requires you to write more code to free the colors when you are done with them.
>
> Here is what you could have done:
>
> std::vector<unsigned> colorList;
>
> This way:
>
> * You don't need to allocate the list using new(). 
> * You don't need to allocate list members using new(). 
> * You don't have to worry about catching exceptions in order to delete the list pointer you allocated using new(), since the destructor of the vector will automatically get called when it goes out of scope.
> * You use far less memory and your code is a lot simpler and a lot faster.
> * Since the type is not typedef-ed by an obscure name, another programmer would know immediately what data structure he is dealing with (a vector of integers).
>
> Also, you did not follow the style of the rest of the code by declaring all-caps data types, in defiance of pretty much every single type in the rest of the project...
>
> Thanks,
> Traian
>
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
> Sent: Friday, May 08, 2009 8:29 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] Some code fixes for RFC 60
>
> Thanks for the input. Much appreciated.
>
> Added the delete.
>
> Why did I use pointers?
> I did use pointers as this leaves more flexibility for later improvements.
> Also In the beginning I was not sure if the map object might be shared 
> between threads or sessions.
> I can imagine some use cases where this might be an issue.... at that 
> stage different colorpalettes need to be handled and pointers would be 
> more flexible.
>
> To notice an eventual leak in the assignment I added the assert 
> statement....
> so its an odd coding but a destructive assignment does not leak it 
> breaks ;) 
> This is like a runtime check to understand whats happening in a 
> multithreaded server.
> So its the same  for now.
>
> I added those things now.... the Coordsys error message (#980) is also 
> included.
> I  commented out the experimental exception throw in StylizeLayers...
>
> Cheers,
> UV
>   
_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals