[mapguide-internals] RFC90 metatiling progress

Fri Jul 22 14:31:17 EDT 2011

Hi UV,

Including rendering service in tile service is ok.  Basically, the services dependencies are structured according to which services build on other services.  The tile service does render so it reasonable to have that dependency.

Resource service - Depends on no services
Feature service - Depends on Resource service
Mapping service - Depends on Feature and Resource services
Rendering service - Depends on Mapping, Feature, Resource services
Tile service - Depends on all of the above

As another example, including Tile service in Rendering service would be bad since the Rendering service should be able to "do its job" without the Tile service.

I took a look at the submission on rfc90.2.  It looks better.  Here are some additional comments:

MgDev/build.bat
- please put doc generation back in

serverconfig.ini
- please revert changes to serverconfig.ini and only leave the new config parameters in place.
  It may affect installer substitution.

MgServerRenderingService::RenderTileFromMetaTile
- What is the purpose of MyByteSource?
- auto_ptr<MgByte> shoulbe be replaced by Ptr<MgByte> since it derives from MgDisposable

ServerTileService.cpp
- error message for large sm_useMetaTiles parameter should be localized and put in mapguide_en.res

ServerTileService.cpp, ServerTileService.h
- Remove commented out code

TileCache.cpp, TileCache.h
- MgTileService is not a singleton.  Ptr<MgTileService> m_tileService is more correct.
- An new instance of MgTileCache is created for each MgTileService.
- Remove commented out code
- Should MgTileCache::LockTile also include the sm_tileMutex guard?
- Is access to the sm_tileMutex guard needed outside of MgTileCache?  If not, move the declaration to the protected section of the class definition.

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: June 29, 2011 4:17 AM
To: mapguide-internals at lists.osgeo.org
Subject: [mapguide-internals] RFC90 metatiling progress

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.
>

_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals