[mapguide-internals] RFC90 metatiling progress

Thu Jul 21 00:39:53 EDT 2011

Can anyone shed some insight on the inter-service dependencies question?

On Wed, Jun 29, 2011 at 8:16 PM, UV <uvwild at googlemail.com> wrote:

> 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@**lists.osgeo.org<mapguide-internals-bounces at lists.osgeo.org>[mailto:
>> mapguide-internals-**bounces at lists.osgeo.org<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
>>> ------------------
>>> - TileServicePropertyUseMetaTile**s, 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 MgConfigurationPropertyValueIs**OutOfRange
>>> <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.
>>
>>
> ______________________________**_________________
> mapguide-internals mailing list
> mapguide-internals at lists.**osgeo.org <mapguide-internals at lists.osgeo.org>
> http://lists.osgeo.org/**mailman/listinfo/mapguide-**internals<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