[mapguide-internals] RFC90 metatiling progress

[UV]

Hello,

I commited my changes to the new branch and added some comments to the 
rfc90 page in Trac.
If someone cares to have a look I am happy to integrate those remarks 
for my final contribution.

I integrated most of Trevors suggestions below, more to come.

I would appreciate a general statement regarding inter-service dependencies.
Adding the rfc90 functionality forced me to include the rendering 
service from the tile service to access the additional code.

I am open for suggestions how to avoid such dependencies without 
duplicating code unnecessarily.

Kind Regards,
UV

On 6/11/2010 18:22, Trevor Wekel wrote:
> Hi UV,
>
> A couple of comments below.  We can schedule time for a chat session.  When are you available?
>
> GetTileCoords() move to MgMap
> - Other use cases include programmatic calculation of tile location by external applications.
>
> DetectLockFile() move to MgTileCache
> - The locking mechanism (whatever it happens to be) is for tiles and should be encapsulated/hidden accordingly.
> - I will also look into ACE::Conditions.  It's always better to have ACE do the work for us...
>
>
> Regards,
> Trevor
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
> Sent: November 5, 2010 4:36 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] Initial review on RFC90 code
>
> Hi Trevor,
>
> I finally managed to get back to this and are currently working to
> resolve some of the mentioned issues.
> Thanks a lot for the comments.
>
> However, some comments are not 100% clear to me or there is a config
> reason behind.
> I think a direct chat session would be a most efficient way to sort out
> the remaining issues.
> I am on CET now and have time for a few days between changing jobs.
>
> Comments on each issue inline below.
>
> On 27/04/2010 20:09, Trevor Wekel wrote:
>> Hi UV,
>>
>> Here are some comments from my initial review of the RFC 90 code.  I will do a follow up review once the RFC has been updated and code changes have been made.
>>
>> Thank you,
>> Trevor
>>
>>
>> General Comments
>> ----------------
>> ----------------
>>
>> RFC 90 documentation in Trac is incomplete.  All serverconfig.ini parameters and new function signatures should be documented.  A general algorithm overview would also be good.
>>
>> Based on my initial review, the following items should be documented:
>> - useMetaTiles and LockMethod serverconfig.ini parameters.
>> - Behavioural modification to AGGRenderer::Save and AGGImageIO.cpp
>> - New MgTileCache behaviour (see below, I believe some refactoring would be appropriate)
>>
>> File specific comments
>> ----------------------
>> ----------------------
>>
>> RenderingService.h
>> ProxyRenderingService.h
>> -----------------------
>> - new RenderFromMetaTile signature commented out, should it be removed?
>>
>>
>> ConfigProperties.cpp
>> ConfigProperties.h
>> ------------------
>> - TileServicePropertyUseMetaTiles, TileServicePropertyLockMethod should be doc'd in RFC.
>> - New properties should be added to MgConfigProperties::sm_cviTileServiceProperties array
> DONE
>> AGGRenderer.cpp
>> ---------------
>> - "META" image format could be defined in MgImageFormats as an INTERNAL_API
>> - new ::SaveMethod uses format = "META".  I am not 100% clear on how this all fits together.  Some algorithm documetation in the RFC would be helpful.
>> - Can assert be replaced by a throw (MgInvalidArgumentException)?
> added imageformat and mimetype for this
>> AggImageIO.cpp
>> --------------
>> - Please remove commented out code.
>> ServerRenderingService.vcproj
>> -----------------------------
>> - Rendering Service should not depend on Tile service headers.
> I know, but I needed these dependencies due  to the way the metatile is
> being passed around.  As mentioned above I would appreciate a brief
> discussion regarding a different approach.
>> ServerRenderingService.cpp
>> --------------------------
>> - GetTileCoords()
>> What purpose does tileColOffset serve?
>> Could this method be moved into MgMap as an INTERNAL_API method which takes tileWidth and tileHeight as parameters?
> moved to MgMap
>
> - RenderTileFromMetaTile()
>> Can asserts be replaced by MgInvalidArgumentException, MgOutOfRangeException?
>> Should bsimp->GetLength() be tested for 0<   size<   2^31 before casting?
>> Why is MyByteSource required?  The direct cast is fragile.
> I am open for suggestions how to do it differently!
> I tried to avoid touching Foundation classes, but those are not really
> made for this so I had to use a cast.
> I was juggling quite a bit with parameters and types to find the least
> intrusive way to pass the metatile bitmap back from the RenderingService
> back to the TileService to be placed into the separate subtiles.
>
>> ServerTileService.cpp
>> ---------------------
>> - RFC documentation for sm_creationCutoffTime, sm_pollingInterval, sm_useMetaTiles, sm_lockMethod
> OK coming up
>> - sm_creationCutoffTime and sm_pollingInterval should use same time base.
> ok, but specifying cutofftime as 120000 seems also impractical.
>> - MgServerTileService::MgServerTileService() : MgTileService()
>> METAMAXDIM should be moved to configuration parameters MG_CONFIG_MAX_METATILE_DIM
>> Error message should use MgConfigurationPropertyValueIsOutOfRange
>> <sstream>   use could be dropped.
> normal strings are typedef wstring STRING;
> This is debug code which IMHO should not clutter the important logic, so
> I chose the shortest way to code such a concatenation of values
> by using std::wstringstream. I can add a macro to FoundationDefs.h next
> to STRING if thats better.
> Is there any other way to do a less verbose string concatenation?
added config variables
>> - DetectLockFile()
>> ------------------
>> This functionality should be moved into MgTileCache and probably renamed to WaitForTileLock.
>> Is a subsecond sleep really necessary?
> As a performing tuning measure I figured it would have some impact to
> have more fine grained control.  Thinking about multicore systems>  4
> cores this could speed up things significantly for small tiles. But I am
> not insisting on this and personally more into the ACE::Conditions.
>> ACE_OS::sleep (const ACE_Time_Value&tv) may simplify code
> thats whats used on line 144
>> - GetTile()
>> -----------
>> Since there is no signature change to the public API, APIVERSION 1.2 is not really required.
> I added those comments to understand the sparsely documented code
> better. If its wrong I can remove it. Otherwise its documentation.
>> - GetSingleTile()
>> -----------------
>> MgFoundBlockingLockFile must be added to resource string file(s) in MapGuideCommon/Resources
> done
>> - GetMetaTile()
>> ---------------
>> Locking methods should be documented in the RFC.
>> Algorithm can be simplified by only generating a lock for the left top corner.  Meta tiling will be always on or always off.  For future work, each map will also be meta tiled or not.
>> I believe there is too much special case locking logic coded here.  The locking logic should be moved into MgTileCache (LockTile(), UnlockTile(), WaitForTileLock()).
>> All error messages must be defined in MapGuideCommon/Resources
>> <sstream>   use could be dropped.
> added some text to Trac rfc90 page
>> TileCache.h
>> -----------
>> - GetLockMap()
>> Lock map structure should not be directly exposed using GetLockMap().  LockTile() and UnlockTile() methods should be added.  New methods should hide sm_lockMethod details from the rest of the code.
> I gonna have a look at this refactoring.
>